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

[PATCH 1/5] power: supply: ds2782: Switch to 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 idr_*.

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

diff --git a/drivers/power/supply/ds2782_battery.c b/drivers/power/supply/ds2782_battery.c
index 85aa9c465aa4e..10428d781c18b 100644
--- a/drivers/power/supply/ds2782_battery.c
+++ b/drivers/power/supply/ds2782_battery.c
@@ -63,8 +63,7 @@ struct ds278x_info {
 	int			status;		/* State Of Charge */
 };
 
-static DEFINE_IDR(battery_id);
-static DEFINE_MUTEX(battery_lock);
+static DEFINE_IDA(battery_id);
 
 static inline int ds278x_read_reg(struct ds278x_info *info, int reg, u8 *val)
 {
@@ -322,9 +321,7 @@ static void ds278x_battery_remove(struct i2c_client *client)
 	kfree(info->battery_desc.name);
 	kfree(info);
 
-	mutex_lock(&battery_lock);
-	idr_remove(&battery_id, id);
-	mutex_unlock(&battery_lock);
+	ida_free(&battery_id, id);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -387,12 +384,9 @@ static int ds278x_battery_probe(struct i2c_client *client)
 	}
 
 	/* Get an ID for this battery */
-	mutex_lock(&battery_lock);
-	ret = idr_alloc(&battery_id, client, 0, 0, GFP_KERNEL);
-	mutex_unlock(&battery_lock);
-	if (ret < 0)
-		goto fail_id;
-	num = ret;
+	num = ida_alloc(&battery_id, GFP_KERNEL);
+	if (num < 0)
+		return num;
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
 	if (!info) {
@@ -439,10 +433,7 @@ static int ds278x_battery_probe(struct i2c_client *client)
 fail_name:
 	kfree(info);
 fail_info:
-	mutex_lock(&battery_lock);
-	idr_remove(&battery_id, num);
-	mutex_unlock(&battery_lock);
-fail_id:
+	ida_free(&battery_id, num);
 	return ret;
 }
 
-- 
2.39.2

[PATCH 2/5] power: supply: ds2782: Free IDA with devm action

[Andrew Davis]

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/ds2782_battery.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/power/supply/ds2782_battery.c b/drivers/power/supply/ds2782_battery.c
index 10428d781c18b..28ad11c8f82de 100644
--- a/drivers/power/supply/ds2782_battery.c
+++ b/drivers/power/supply/ds2782_battery.c
@@ -57,7 +57,6 @@ struct ds278x_info {
 	struct power_supply_desc	battery_desc;
 	const struct ds278x_battery_ops *ops;
 	struct delayed_work	bat_work;
-	int			id;
 	int                     rsns;
 	int			capacity;
 	int			status;		/* State Of Charge */
@@ -314,14 +313,11 @@ static void ds278x_power_supply_init(struct power_supply_desc *battery)
 static void ds278x_battery_remove(struct i2c_client *client)
 {
 	struct ds278x_info *info = i2c_get_clientdata(client);
-	int id = info->id;
 
 	power_supply_unregister(info->battery);
 	cancel_delayed_work_sync(&info->bat_work);
 	kfree(info->battery_desc.name);
 	kfree(info);
-
-	ida_free(&battery_id, id);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -365,6 +361,13 @@ static const struct ds278x_battery_ops ds278x_ops[] = {
 	}
 };
 
+static void ds278x_free_ida(void *data)
+{
+	int num = (uintptr_t)data;
+
+	ida_free(&battery_id, num);
+}
+
 static int ds278x_battery_probe(struct i2c_client *client)
 {
 	const struct i2c_device_id *id = i2c_client_get_device_id(client);
@@ -387,12 +390,13 @@ static int ds278x_battery_probe(struct i2c_client *client)
 	num = ida_alloc(&battery_id, GFP_KERNEL);
 	if (num < 0)
 		return num;
+	ret = devm_add_action_or_reset(&client->dev, ds278x_free_ida, (void *)(uintptr_t)num);
+	if (ret)
+		return ret;
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
-	if (!info) {
-		ret = -ENOMEM;
-		goto fail_info;
-	}
+	if (!info)
+		return -ENOMEM;
 
 	info->battery_desc.name = kasprintf(GFP_KERNEL, "%s-%d",
 					    client->name, num);
@@ -406,7 +410,6 @@ static int ds278x_battery_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, info);
 	info->client = client;
-	info->id = num;
 	info->ops  = &ds278x_ops[id->driver_data];
 	ds278x_power_supply_init(&info->battery_desc);
 	psy_cfg.drv_data = info;
@@ -432,8 +435,6 @@ static int ds278x_battery_probe(struct i2c_client *client)
 	kfree(info->battery_desc.name);
 fail_name:
 	kfree(info);
-fail_info:
-	ida_free(&battery_id, num);
 	return ret;
 }
 
-- 
2.39.2

[PATCH 3/5] power: supply: ds2782: Use devm based memory allocators

[Andrew Davis]

Use device lifecycle managed memory alloc helpers. This helps
prevent mistakes like freeing out of order in cleanup functions and
forgetting to free on all error paths.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/power/supply/ds2782_battery.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/power/supply/ds2782_battery.c b/drivers/power/supply/ds2782_battery.c
index 28ad11c8f82de..a72d8c26650d7 100644
--- a/drivers/power/supply/ds2782_battery.c
+++ b/drivers/power/supply/ds2782_battery.c
@@ -316,8 +316,6 @@ static void ds278x_battery_remove(struct i2c_client *client)
 
 	power_supply_unregister(info->battery);
 	cancel_delayed_work_sync(&info->bat_work);
-	kfree(info->battery_desc.name);
-	kfree(info);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -394,16 +392,14 @@ static int ds278x_battery_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
 
-	info->battery_desc.name = kasprintf(GFP_KERNEL, "%s-%d",
-					    client->name, num);
-	if (!info->battery_desc.name) {
-		ret = -ENOMEM;
-		goto fail_name;
-	}
+	info->battery_desc.name = devm_kasprintf(&client->dev, GFP_KERNEL,
+						 "%s-%d", client->name, num);
+	if (!info->battery_desc.name)
+		return -ENOMEM;
 
 	if (id->driver_data == DS2786)
 		info->rsns = pdata->rsns;
@@ -423,19 +419,12 @@ static int ds278x_battery_probe(struct i2c_client *client)
 					      &info->battery_desc, &psy_cfg);
 	if (IS_ERR(info->battery)) {
 		dev_err(&client->dev, "failed to register battery\n");
-		ret = PTR_ERR(info->battery);
-		goto fail_register;
+		return PTR_ERR(info->battery);
 	} else {
 		schedule_delayed_work(&info->bat_work, DS278x_DELAY);
 	}
 
 	return 0;
-
-fail_register:
-	kfree(info->battery_desc.name);
-fail_name:
-	kfree(info);
-	return ret;
 }
 
 static const struct i2c_device_id ds278x_id[] = {
-- 
2.39.2

[PATCH 4/5] power: supply: ds2782: 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/ds2782_battery.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/ds2782_battery.c b/drivers/power/supply/ds2782_battery.c
index a72d8c26650d7..ea687b9703314 100644
--- a/drivers/power/supply/ds2782_battery.c
+++ b/drivers/power/supply/ds2782_battery.c
@@ -314,7 +314,6 @@ static void ds278x_battery_remove(struct i2c_client *client)
 {
 	struct ds278x_info *info = i2c_get_clientdata(client);
 
-	power_supply_unregister(info->battery);
 	cancel_delayed_work_sync(&info->bat_work);
 }
 
@@ -415,8 +414,9 @@ static int ds278x_battery_probe(struct i2c_client *client)
 
 	INIT_DELAYED_WORK(&info->bat_work, ds278x_bat_work);
 
-	info->battery = power_supply_register(&client->dev,
-					      &info->battery_desc, &psy_cfg);
+	info->battery = devm_power_supply_register(&client->dev,
+						   &info->battery_desc,
+						   &psy_cfg);
 	if (IS_ERR(info->battery)) {
 		dev_err(&client->dev, "failed to register battery\n");
 		return PTR_ERR(info->battery);
-- 
2.39.2

[PATCH 5/5] power: supply: ds2782: Use devm_delayed_work_autocancel() helper

[Andrew Davis]

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

Note we move this to after the registering the power supply so that
the cancel is called before unregistering.

This was the last thing the .remove() function did, so remove that too.

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

diff --git a/drivers/power/supply/ds2782_battery.c b/drivers/power/supply/ds2782_battery.c
index ea687b9703314..cae95d35d3980 100644
--- a/drivers/power/supply/ds2782_battery.c
+++ b/drivers/power/supply/ds2782_battery.c
@@ -11,6 +11,7 @@
  * UEvent sending added by Evgeny Romanov <romanov@neurosoft.ru>
  */
 
+#include <linux/devm-helpers.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/types.h>
@@ -310,13 +311,6 @@ static void ds278x_power_supply_init(struct power_supply_desc *battery)
 	battery->external_power_changed	= NULL;
 }
 
-static void ds278x_battery_remove(struct i2c_client *client)
-{
-	struct ds278x_info *info = i2c_get_clientdata(client);
-
-	cancel_delayed_work_sync(&info->bat_work);
-}
-
 #ifdef CONFIG_PM_SLEEP
 
 static int ds278x_suspend(struct device *dev)
@@ -412,18 +406,19 @@ static int ds278x_battery_probe(struct i2c_client *client)
 	info->capacity = 100;
 	info->status = POWER_SUPPLY_STATUS_FULL;
 
-	INIT_DELAYED_WORK(&info->bat_work, ds278x_bat_work);
-
 	info->battery = devm_power_supply_register(&client->dev,
 						   &info->battery_desc,
 						   &psy_cfg);
 	if (IS_ERR(info->battery)) {
 		dev_err(&client->dev, "failed to register battery\n");
 		return PTR_ERR(info->battery);
-	} else {
-		schedule_delayed_work(&info->bat_work, DS278x_DELAY);
 	}
 
+	ret = devm_delayed_work_autocancel(&client->dev, &info->bat_work, ds278x_bat_work);
+	if (ret)
+		return ret;
+	schedule_delayed_work(&info->bat_work, DS278x_DELAY);
+
 	return 0;
 }
 
@@ -440,7 +435,6 @@ static struct i2c_driver ds278x_battery_driver = {
 		.pm	= &ds278x_battery_pm_ops,
 	},
 	.probe		= ds278x_battery_probe,
-	.remove		= ds278x_battery_remove,
 	.id_table	= ds278x_id,
 };
 module_i2c_driver(ds278x_battery_driver);
-- 
2.39.2

Re: [PATCH 1/5] power: supply: ds2782: Switch to simpler IDA interface

[Sebastian Reichel]

On Mon, 02 Dec 2024 15:15:15 -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 idr_*.
> 
> 

Applied, thanks!

[1/5] power: supply: ds2782: Switch to simpler IDA interface
      commit: bea4395a04d2602e72f550e795c15e98e557b779
[2/5] power: supply: ds2782: Free IDA with devm action
      commit: fd647cc2cb73e8a6403f9c59fd7956f68f2e6b35
[3/5] power: supply: ds2782: Use devm based memory allocators
      commit: 1481f9f39091b95aec52553a9652d84a827a6004
[4/5] power: supply: ds2782: Use devm_power_supply_register() helper
      commit: 8571178e9adf3128d70d14359b965f370cfd522d
[5/5] power: supply: ds2782: Use devm_delayed_work_autocancel() helper
      commit: 1c44832979a70570f2e652013877c7b15000494e

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