[PATCH] power: supply: axp288-charger: Optimize register reading method

* [PATCH] power: supply: axp288-charger: Optimize register reading method
@ 2021-10-12  5:45 Kate Hsuan
  2021-10-12  7:52 ` Hans de Goede
  0 siblings, 1 reply; 3+ messages in thread
From: Kate Hsuan @ 2021-10-12  5:45 UTC (permalink / raw)
  To: Sebastian Reichel, Hans de Goede, hen-Yu Tsai
  Cc: Kate Hsuan, linux-pm, linux-kernel

The original implementation access the charger the same register value
several times to get the charger status, such as online, enabled, and
bus limits. It takes a long time and bandwidth for every "status get"
operation. 

To reduce the access of the register and save bandwidth, this commit
integrated every read operation into only one "register value get" 
operation and cache them in the variables. Once the "get properties"
is requested from the user space, the cached information can be returned
immediately.

I2C access between Linux kernel and P-Unit is improved by explicitly taking
semaphore once for the entire set of register accesses in the new
axp288_charger_usb_update_property() function. The I2C-Bus to the XPower
AXP288 is shared between the Linux kernel and SoCs P-Unit. The P-Unit
has a semaphore which the kernel must "lock" before it may use the bus.
If not explicitly taken by the I2C-Driver, then this semaphore is
automatically taken by the I2C-bus-driver for each I2C-transfer. In
other words, the semaphore will be locked and released several times for
entire set of register accesses.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 150 +++++++++++++++++---------
 1 file changed, 99 insertions(+), 51 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index b9553be9bed5..fd4983c98fd9 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -22,6 +22,7 @@
 #include <linux/mfd/axp20x.h>
 #include <linux/extcon.h>
 #include <linux/dmi.h>
+#include <asm/iosf_mbi.h>
 
 #define PS_STAT_VBUS_TRIGGER		BIT(0)
 #define PS_STAT_BAT_CHRG_DIR		BIT(2)
@@ -95,6 +96,8 @@
 #define CV_4200MV			4200	/* 4200mV */
 #define CV_4350MV			4350	/* 4350mV */
 
+#define AXP288_REG_UPDATE_INTERVAL	(60 * HZ)
+
 #define AXP288_EXTCON_DEV_NAME		"axp288_extcon"
 #define USB_HOST_EXTCON_HID		"INT3496"
 #define USB_HOST_EXTCON_NAME		"INT3496:00"
@@ -118,6 +121,7 @@ struct axp288_chrg_info {
 	struct regmap_irq_chip_data *regmap_irqc;
 	int irq[CHRG_INTR_END];
 	struct power_supply *psy_usb;
+	struct mutex lock;
 
 	/* OTG/Host mode */
 	struct {
@@ -138,6 +142,12 @@ struct axp288_chrg_info {
 	int cv;
 	int max_cc;
 	int max_cv;
+
+	unsigned long last_updated;
+	unsigned int input_status;
+	unsigned int op_mode;
+	unsigned int backend_control;
+	bool valid;
 };
 
 static inline int axp288_charger_set_cc(struct axp288_chrg_info *info, int cc)
@@ -197,11 +207,8 @@ static inline int axp288_charger_set_cv(struct axp288_chrg_info *info, int cv)
 static int axp288_charger_get_vbus_inlmt(struct axp288_chrg_info *info)
 {
 	unsigned int val;
-	int ret;
 
-	ret = regmap_read(info->regmap, AXP20X_CHRG_BAK_CTRL, &val);
-	if (ret < 0)
-		return ret;
+	val = info->backend_control;
 
 	val >>= CHRG_VBUS_ILIM_BIT_POS;
 	switch (val) {
@@ -297,55 +304,34 @@ static int axp288_charger_enable_charger(struct axp288_chrg_info *info,
 
 static int axp288_charger_is_present(struct axp288_chrg_info *info)
 {
-	int ret, present = 0;
-	unsigned int val;
-
-	ret = regmap_read(info->regmap, AXP20X_PWR_INPUT_STATUS, &val);
-	if (ret < 0)
-		return ret;
+	int present = 0;
 
-	if (val & PS_STAT_VBUS_PRESENT)
+	if (info->input_status & PS_STAT_VBUS_PRESENT)
 		present = 1;
 	return present;
 }
 
 static int axp288_charger_is_online(struct axp288_chrg_info *info)
 {
-	int ret, online = 0;
-	unsigned int val;
-
-	ret = regmap_read(info->regmap, AXP20X_PWR_INPUT_STATUS, &val);
-	if (ret < 0)
-		return ret;
+	int online = 0;
 
-	if (val & PS_STAT_VBUS_VALID)
+	if (info->input_status & PS_STAT_VBUS_VALID)
 		online = 1;
 	return online;
 }
 
 static int axp288_get_charger_health(struct axp288_chrg_info *info)
 {
-	int ret, pwr_stat, chrg_stat;
 	int health = POWER_SUPPLY_HEALTH_UNKNOWN;
-	unsigned int val;
 
-	ret = regmap_read(info->regmap, AXP20X_PWR_INPUT_STATUS, &val);
-	if ((ret < 0) || !(val & PS_STAT_VBUS_PRESENT))
+	if (!(info->input_status & PS_STAT_VBUS_PRESENT))
 		goto health_read_fail;
-	else
-		pwr_stat = val;
 
-	ret = regmap_read(info->regmap, AXP20X_PWR_OP_MODE, &val);
-	if (ret < 0)
-		goto health_read_fail;
-	else
-		chrg_stat = val;
-
-	if (!(pwr_stat & PS_STAT_VBUS_VALID))
+	if (!(info->input_status & PS_STAT_VBUS_VALID))
 		health = POWER_SUPPLY_HEALTH_DEAD;
-	else if (chrg_stat & CHRG_STAT_PMIC_OTP)
+	else if (info->op_mode & CHRG_STAT_PMIC_OTP)
 		health = POWER_SUPPLY_HEALTH_OVERHEAT;
-	else if (chrg_stat & CHRG_STAT_BAT_SAFE_MODE)
+	else if (info->op_mode & CHRG_STAT_BAT_SAFE_MODE)
 		health = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
 	else
 		health = POWER_SUPPLY_HEALTH_GOOD;
@@ -362,30 +348,86 @@ static int axp288_charger_usb_set_property(struct power_supply *psy,
 	int ret = 0;
 	int scaled_val;
 
+	mutex_lock(&info->lock);
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
 		scaled_val = min(val->intval, info->max_cc);
 		scaled_val = DIV_ROUND_CLOSEST(scaled_val, 1000);
 		ret = axp288_charger_set_cc(info, scaled_val);
-		if (ret < 0)
+		if (ret < 0) {
 			dev_warn(&info->pdev->dev, "set charge current failed\n");
+			goto out;
+		}
 		break;
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
 		scaled_val = min(val->intval, info->max_cv);
 		scaled_val = DIV_ROUND_CLOSEST(scaled_val, 1000);
 		ret = axp288_charger_set_cv(info, scaled_val);
-		if (ret < 0)
+		if (ret < 0) {
 			dev_warn(&info->pdev->dev, "set charge voltage failed\n");
+			goto out;
+		}
 		break;
 	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
 		ret = axp288_charger_set_vbus_inlmt(info, val->intval);
-		if (ret < 0)
+		if (ret < 0) {
 			dev_warn(&info->pdev->dev, "set input current limit failed\n");
+			goto out;
+		}
+		info->valid = false;
 		break;
 	default:
 		ret = -EINVAL;
 	}
 
+out:
+	mutex_unlock(&info->lock);
+	return ret;
+}
+
+static int axp288_charger_reg_readb(struct axp288_chrg_info *info, int reg, unsigned int *ret_val)
+{
+	int ret;
+
+	ret = regmap_read(info->regmap, reg, ret_val);
+	if (ret < 0) {
+		dev_err(&info->pdev->dev, "Error %d on reading value from register 0x%04x\n",
+			ret,
+			reg);
+		return ret;
+	}
+	return 0;
+}
+
+static int axp288_charger_usb_update_property(struct axp288_chrg_info *info)
+{
+	int ret = 0;
+
+	if (info->valid && time_before(jiffies, info->last_updated + AXP288_REG_UPDATE_INTERVAL))
+		return 0;
+
+	dev_dbg(&info->pdev->dev, "Charger updating register values...\n");
+
+	ret = iosf_mbi_block_punit_i2c_access();
+	if (ret < 0)
+		return ret;
+
+	ret = axp288_charger_reg_readb(info, AXP20X_PWR_INPUT_STATUS, &info->input_status);
+	if (ret < 0)
+		goto out;
+
+	ret = axp288_charger_reg_readb(info, AXP20X_PWR_OP_MODE, &info->op_mode);
+	if (ret < 0)
+		goto out;
+
+	ret = axp288_charger_reg_readb(info, AXP20X_CHRG_BAK_CTRL, &info->backend_control);
+	if (ret < 0)
+		goto out;
+
+	info->last_updated = jiffies;
+	info->valid = true;
+out:
+	iosf_mbi_unblock_punit_i2c_access();
 	return ret;
 }
 
@@ -396,6 +438,11 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
 	struct axp288_chrg_info *info = power_supply_get_drvdata(psy);
 	int ret;
 
+	mutex_lock(&info->lock);
+	ret = axp288_charger_usb_update_property(info);
+	if (ret < 0)
+		goto out;
+
 	switch (psp) {
 	case POWER_SUPPLY_PROP_PRESENT:
 		/* Check for OTG case first */
@@ -403,10 +450,7 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
 			val->intval = 0;
 			break;
 		}
-		ret = axp288_charger_is_present(info);
-		if (ret < 0)
-			return ret;
-		val->intval = ret;
+		val->intval = axp288_charger_is_present(info);
 		break;
 	case POWER_SUPPLY_PROP_ONLINE:
 		/* Check for OTG case first */
@@ -414,10 +458,7 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
 			val->intval = 0;
 			break;
 		}
-		ret = axp288_charger_is_online(info);
-		if (ret < 0)
-			return ret;
-		val->intval = ret;
+		val->intval = axp288_charger_is_online(info);
 		break;
 	case POWER_SUPPLY_PROP_HEALTH:
 		val->intval = axp288_get_charger_health(info);
@@ -435,16 +476,15 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
 		val->intval = info->max_cv * 1000;
 		break;
 	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
-		ret = axp288_charger_get_vbus_inlmt(info);
-		if (ret < 0)
-			return ret;
-		val->intval = ret;
+		val->intval = axp288_charger_get_vbus_inlmt(info);
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
 
-	return 0;
+out:
+	mutex_unlock(&info->lock);
+	return ret;
 }
 
 static int axp288_charger_property_is_writeable(struct power_supply *psy,
@@ -540,7 +580,9 @@ static irqreturn_t axp288_charger_irq_thread_handler(int irq, void *dev)
 		dev_warn(&info->pdev->dev, "Spurious Interrupt!!!\n");
 		goto out;
 	}
-
+	mutex_lock(&info->lock);
+	info->valid = false;
+	mutex_unlock(&info->lock);
 	power_supply_changed(info->psy_usb);
 out:
 	return IRQ_HANDLED;
@@ -613,6 +655,9 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 	if (!(val & PS_STAT_VBUS_VALID)) {
 		dev_dbg(&info->pdev->dev, "USB charger disconnected\n");
 		axp288_charger_enable_charger(info, false);
+		mutex_lock(&info->lock);
+		info->valid = false;
+		mutex_unlock(&info->lock);
 		power_supply_changed(info->psy_usb);
 		return;
 	}
@@ -644,6 +689,9 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 		dev_err(&info->pdev->dev,
 			"error setting current limit (%d)\n", ret);
 
+	mutex_lock(&info->lock);
+	info->valid = false;
+	mutex_unlock(&info->lock);
 	power_supply_changed(info->psy_usb);
 }
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread