[PATCH 1/5] power: supply: bq27xxx: Switch to a simpler IDA interface

[PATCH 1/5] power: supply: bq27xxx: Switch to a simpler IDA interface

[Andrew Davis]

We don't need to specify any ranges when allocating IDs so we can switch
to ida_alloc() and ida_free() instead of the ida_simple_ counterparts.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/power/supply/bq27xxx_battery_i2c.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index 3a1798b0c1a79..86ce13a8ab9dd 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -13,8 +13,7 @@
 
 #include <linux/power/bq27xxx_battery.h>
 
-static DEFINE_IDR(battery_id);
-static DEFINE_MUTEX(battery_mutex);
+static DEFINE_IDA(battery_id);
 
 static irqreturn_t bq27xxx_battery_irq_handler_thread(int irq, void *data)
 {
@@ -145,9 +144,7 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client)
 	int num;
 
 	/* Get new ID for the new battery device */
-	mutex_lock(&battery_mutex);
-	num = idr_alloc(&battery_id, client, 0, 0, GFP_KERNEL);
-	mutex_unlock(&battery_mutex);
+	num = ida_alloc(&battery_id, GFP_KERNEL);
 	if (num < 0)
 		return num;
 
@@ -198,9 +195,7 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client)
 	ret = -ENOMEM;
 
 err_failed:
-	mutex_lock(&battery_mutex);
-	idr_remove(&battery_id, num);
-	mutex_unlock(&battery_mutex);
+	ida_free(&battery_id, num);
 
 	return ret;
 }
@@ -212,9 +207,7 @@ static void bq27xxx_battery_i2c_remove(struct i2c_client *client)
 	free_irq(client->irq, di);
 	bq27xxx_battery_teardown(di);
 
-	mutex_lock(&battery_mutex);
-	idr_remove(&battery_id, di->id);
-	mutex_unlock(&battery_mutex);
+	ida_free(&battery_id, di->id);
 }
 
 static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
-- 
2.39.2

[PATCH 2/5] power: supply: bq27xxx: Add devm action to free IDA

[Andrew Davis]

Use a device lifecycle managed action to free the IDA. This helps
prevent mistakes like freeing out of order in cleanup functions and
forgetting to free on error paths.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/power/supply/bq27xxx_battery_i2c.c | 35 +++++++++++-----------
 include/linux/power/bq27xxx_battery.h      |  1 -
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index 86ce13a8ab9dd..019f29d13d28a 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -135,28 +135,39 @@ static int bq27xxx_battery_i2c_bulk_write(struct bq27xxx_device_info *di,
 	return 0;
 }
 
+static void bq27xxx_battery_i2c_devm_ida_free(void *data)
+{
+	int num = (long)data;
+
+	ida_free(&battery_id, num);
+}
+
 static int bq27xxx_battery_i2c_probe(struct i2c_client *client)
 {
 	const struct i2c_device_id *id = i2c_client_get_device_id(client);
 	struct bq27xxx_device_info *di;
 	int ret;
 	char *name;
-	int num;
+	long num;
 
 	/* Get new ID for the new battery device */
 	num = ida_alloc(&battery_id, GFP_KERNEL);
 	if (num < 0)
 		return num;
+	ret = devm_add_action_or_reset(&client->dev,
+				       bq27xxx_battery_i2c_devm_ida_free,
+				       (void *)num);
+	if (ret)
+		return ret;
 
-	name = devm_kasprintf(&client->dev, GFP_KERNEL, "%s-%d", id->name, num);
+	name = devm_kasprintf(&client->dev, GFP_KERNEL, "%s-%ld", id->name, num);
 	if (!name)
-		goto err_mem;
+		return -ENOMEM;
 
 	di = devm_kzalloc(&client->dev, sizeof(*di), GFP_KERNEL);
 	if (!di)
-		goto err_mem;
+		return -ENOMEM;
 
-	di->id = num;
 	di->dev = &client->dev;
 	di->chip = id->driver_data;
 	di->name = name;
@@ -168,7 +179,7 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client)
 
 	ret = bq27xxx_battery_setup(di);
 	if (ret)
-		goto err_failed;
+		return ret;
 
 	/* Schedule a polling after about 1 min */
 	schedule_delayed_work(&di->work, 60 * HZ);
@@ -185,19 +196,11 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client)
 				"Unable to register IRQ %d error %d\n",
 				client->irq, ret);
 			bq27xxx_battery_teardown(di);
-			goto err_failed;
+			return ret;
 		}
 	}
 
 	return 0;
-
-err_mem:
-	ret = -ENOMEM;
-
-err_failed:
-	ida_free(&battery_id, num);
-
-	return ret;
 }
 
 static void bq27xxx_battery_i2c_remove(struct i2c_client *client)
@@ -206,8 +209,6 @@ static void bq27xxx_battery_i2c_remove(struct i2c_client *client)
 
 	free_irq(client->irq, di);
 	bq27xxx_battery_teardown(di);
-
-	ida_free(&battery_id, di->id);
 }
 
 static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 7d8025fb74b70..b9e5bd2b42d36 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -61,7 +61,6 @@ struct bq27xxx_reg_cache {
 
 struct bq27xxx_device_info {
 	struct device *dev;
-	int id;
 	enum bq27xxx_chip chip;
 	u32 opts;
 	const char *name;
-- 
2.39.2

[PATCH 3/5] power: supply: bq27xxx: Use devm to free device mutex

[Andrew Davis]

Use a device lifecycle managed action to free the device mutex.
This helps prevent mistakes like freeing out of order in cleanup
functions and forgetting to free on error paths.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/power/supply/bq27xxx_battery.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 1c4a9d1377442..d3b6327b16b56 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -2101,6 +2101,13 @@ static void bq27xxx_external_power_changed(struct power_supply *psy)
 	mod_delayed_work(system_wq, &di->work, HZ / 2);
 }
 
+static void bq27xxx_battery_mutex_destroy(void *data)
+{
+	struct mutex *lock = data;
+
+	mutex_destroy(lock);
+}
+
 int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 {
 	struct power_supply_desc *psy_desc;
@@ -2108,9 +2115,14 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 		.of_node = di->dev->of_node,
 		.drv_data = di,
 	};
+	int ret;
 
 	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
 	mutex_init(&di->lock);
+	ret = devm_add_action_or_reset(di->dev, bq27xxx_battery_mutex_destroy,
+				       &di->lock);
+	if (ret)
+		return ret;
 
 	di->regs       = bq27xxx_chip_data[di->chip].regs;
 	di->unseal_key = bq27xxx_chip_data[di->chip].unseal_key;
@@ -2158,7 +2170,6 @@ void bq27xxx_battery_teardown(struct bq27xxx_device_info *di)
 	cancel_delayed_work_sync(&di->work);
 
 	power_supply_unregister(di->bat);
-	mutex_destroy(&di->lock);
 }
 EXPORT_SYMBOL_GPL(bq27xxx_battery_teardown);
 
-- 
2.39.2

[PATCH 4/5] power: supply: bq27xxx: Use devm_power_supply_register() helper

[Andrew Davis]

Use the device lifecycle managed register function. This helps prevent
mistakes like unregistering out of order in cleanup functions and
forgetting to unregister on error paths.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/power/supply/bq27xxx_battery.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index d3b6327b16b56..2bf5e007f16b2 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -2140,7 +2140,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	psy_desc->get_property = bq27xxx_battery_get_property;
 	psy_desc->external_power_changed = bq27xxx_external_power_changed;
 
-	di->bat = power_supply_register_no_ws(di->dev, psy_desc, &psy_cfg);
+	di->bat = devm_power_supply_register_no_ws(di->dev, psy_desc, &psy_cfg);
 	if (IS_ERR(di->bat))
 		return dev_err_probe(di->dev, PTR_ERR(di->bat),
 				     "failed to register battery\n");
@@ -2168,8 +2168,6 @@ void bq27xxx_battery_teardown(struct bq27xxx_device_info *di)
 	mutex_unlock(&di->lock);
 
 	cancel_delayed_work_sync(&di->work);
-
-	power_supply_unregister(di->bat);
 }
 EXPORT_SYMBOL_GPL(bq27xxx_battery_teardown);
 
-- 
2.39.2

[PATCH 5/5] power: supply: bq27xxx: Move one time design full read out of poll

[Andrew Davis]

This value only needs read once. Move that read into the function
that returns the value to keep the logic all in one place. This
also avoids doing this check every time we read in values in
the device update poll worker.

While here, correct this function's error message.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/power/supply/bq27xxx_battery.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 2bf5e007f16b2..363428530ee60 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1595,17 +1595,24 @@ static inline int bq27xxx_battery_read_fcc(struct bq27xxx_device_info *di)
  * Return the Design Capacity in µAh
  * Or < 0 if something fails.
  */
-static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di)
+static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di,
+				     union power_supply_propval *val)
 {
 	int dcap;
 
+	/* We only have to read charge design full once */
+	if (di->charge_design_full > 0) {
+		val->intval = di->charge_design_full;
+		return 0;
+	}
+
 	if (di->opts & BQ27XXX_O_ZERO)
 		dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, true);
 	else
 		dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, false);
 
 	if (dcap < 0) {
-		dev_dbg(di->dev, "error reading initial last measured discharge\n");
+		dev_dbg(di->dev, "error reading design capacity\n");
 		return dcap;
 	}
 
@@ -1614,7 +1621,12 @@ static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di)
 	else
 		dcap *= 1000;
 
-	return dcap;
+	/* Save for later reads */
+	di->charge_design_full = dcap;
+
+	val->intval = dcap;
+
+	return 0;
 }
 
 /*
@@ -1865,10 +1877,6 @@ static void bq27xxx_battery_update_unlocked(struct bq27xxx_device_info *di)
 		 */
 		if (!(di->opts & BQ27XXX_O_ZERO))
 			bq27xxx_battery_current_and_status(di, NULL, &status, &cache);
-
-		/* We only have to read charge design full once */
-		if (di->charge_design_full <= 0)
-			di->charge_design_full = bq27xxx_battery_read_dcap(di);
 	}
 
 	if ((di->cache.capacity != cache.capacity) ||
@@ -2062,7 +2070,7 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
 		ret = bq27xxx_simple_value(di->cache.charge_full, val);
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
-		ret = bq27xxx_simple_value(di->charge_design_full, val);
+		ret = bq27xxx_battery_read_dcap(di, val);
 		break;
 	/*
 	 * TODO: Implement these to make registers set from
-- 
2.39.2

Re: (subset) [PATCH 1/5] power: supply: bq27xxx: Switch to a simpler IDA interface

[Sebastian Reichel]

On Tue, 23 Jan 2024 09:09:10 -0600, Andrew Davis wrote:
> We don't need to specify any ranges when allocating IDs so we can switch
> to ida_alloc() and ida_free() instead of the ida_simple_ counterparts.
> 
> 

Applied, thanks!

[5/5] power: supply: bq27xxx: Move one time design full read out of poll
      commit: b282c30dad3e10738a4f03043efaff93d9e8de02

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>

Re: (subset) [PATCH 1/5] power: supply: bq27xxx: Switch to a simpler IDA interface

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 499 bytes --]

Hi,

On Sat, Jan 27, 2024 at 01:48:07AM +0100, Sebastian Reichel wrote:
> On Tue, 23 Jan 2024 09:09:10 -0600, Andrew Davis wrote:
> > We don't need to specify any ranges when allocating IDs so we can switch
> > to ida_alloc() and ida_free() instead of the ida_simple_ counterparts.
> 
> Applied, thanks!
> 
> [5/5] power: supply: bq27xxx: Move one time design full read out of poll
>       commit: b282c30dad3e10738a4f03043efaff93d9e8de02

FWIW, I queued all 5 patches.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]