power: supply: ds2782: Switch to simpler IDA interface

PATCH Applied Andrew Davis 2 participants 6 messages Dec 2, 2024 – Dec 5, 2024 Power supply

Share & tools

Andrew Davis

author

2024-12-02 21:15 UTC #0 lore raw

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

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 <[email protected]>

---
 drivers/power/supply/ds2782_battery.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

drivers/power/supply/ds2782_battery.c +6 ~~-15~~

index 85aa9c465aa4e..10428d781c18b 100644
@@ -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;
 }

Signature · 1 line

2.39.2

Andrew Davis

author

2024-12-02 21:15 UTC #1 lore raw

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

This helps prevent mistakes like freeing out of order in cleanup functions and forgetting to free on error paths.

Signed-off-by Andrew Davis <[email protected]>

---
 drivers/power/supply/ds2782_battery.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

drivers/power/supply/ds2782_battery.c +12 ~~-11~~

index 10428d781c18b..28ad11c8f82de 100644
@@ -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;
 }

Signature · 1 line

2.39.2

Andrew Davis

author

2024-12-02 21:15 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/power/supply/ds2782_battery.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

drivers/power/supply/ds2782_battery.c +6 ~~-17~~

index 28ad11c8f82de..a72d8c26650d7 100644
@@ -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[] = {

Signature · 1 line

2.39.2

Andrew Davis

author

2024-12-02 21:15 UTC #3 lore raw

PATCH 4/5 power: supply: ds2782: Use devm_power_supply_register() helper

In reply to 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 <[email protected]>

---
 drivers/power/supply/ds2782_battery.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/power/supply/ds2782_battery.c +3 -3

index a72d8c26650d7..ea687b9703314 100644
@@ -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);

Signature · 1 line

2.39.2

Andrew Davis

author

2024-12-02 21:15 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/power/supply/ds2782_battery.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

drivers/power/supply/ds2782_battery.c +6 ~~-12~~

index ea687b9703314..cae95d35d3980 100644
@@ -11,6 +11,7 @@
  * UEvent sending added by Evgeny Romanov <[email protected]>
  */
 
+#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);

Signature · 1 line

2.39.2

Sebastian Reichel

Applied

2024-12-05 01:36 UTC #5 lore raw

In reply to Andrew Davis

Quoted 4 lines · 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_*.

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,

Signature · 1 line

Sebastian Reichel <[email protected]>