Re: [PATCH v2 04/10] leds: aw200xx: use devm API to cleanup module's resources
From: Christophe Leroy <hidden>
Date: 2023-12-04 23:18:02
Also in:
linux-leds, lkml
Le 04/12/2023 à 19:05, George Stark a écrit :
quoted hunk ↗ jump to hunk
In this driver LEDs are registered using devm_led_classdev_register() so they are automatically unregistered after module's remove() is done. led_classdev_unregister() calls module's led_set_brightness() to turn off the LEDs and that callback uses resources which were destroyed already in module's remove() so use devm API instead of remove(). Signed-off-by: George Stark <redacted> --- drivers/leds/leds-aw200xx.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c index 1d3943f86f7f..b1a097c7c879 100644 --- a/drivers/leds/leds-aw200xx.c +++ b/drivers/leds/leds-aw200xx.c@@ -10,6 +10,7 @@ #include <linux/bitfield.h> #include <linux/bits.h> #include <linux/container_of.h> +#include <linux/devm-helpers.h> #include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/leds.h>@@ -530,6 +531,20 @@ static const struct regmap_config aw200xx_regmap_config = { .disable_locking = true, }; +static void aw200xx_chip_reset_action(void *data) +{ + const struct aw200xx *chip = (struct aw200xx *)data; + + aw200xx_chip_reset(chip);
Same as previous patch, no need to cast data and no need for chip at all, directly do aw200xx_chip_reset(data)
+}
+
+static void aw200xx_disable_action(void *data)
+{
+ const struct aw200xx *chip = (struct aw200xx *)data;
+
+ aw200xx_disable(chip);Same
quoted hunk ↗ jump to hunk
+} + static int aw200xx_probe(struct i2c_client *client) { const struct aw200xx_chipdef *cdef;@@ -568,11 +583,16 @@ static int aw200xx_probe(struct i2c_client *client) aw200xx_enable(chip); + ret = devm_add_action(&client->dev, aw200xx_disable_action, chip); + if (ret) + return ret; + ret = aw200xx_chip_check(chip); if (ret) return ret; - mutex_init(&chip->mutex); + if (devm_mutex_init(&client->dev, &chip->mutex)) + return -ENOMEM;
Why not return value returned by dev_mutex_init() directly ?
quoted hunk ↗ jump to hunk
/* Need a lock now since after call aw200xx_probe_fw, sysfs nodes created */ mutex_lock(&chip->mutex);@@ -581,6 +601,10 @@ static int aw200xx_probe(struct i2c_client *client) if (ret) goto out_unlock; + ret = devm_add_action(&client->dev, aw200xx_chip_reset_action, chip); + if (ret) + goto out_unlock; + ret = aw200xx_probe_fw(&client->dev, chip); if (ret) goto out_unlock;@@ -595,15 +619,6 @@ static int aw200xx_probe(struct i2c_client *client) return ret; } -static void aw200xx_remove(struct i2c_client *client) -{ - struct aw200xx *chip = i2c_get_clientdata(client); - - aw200xx_chip_reset(chip); - aw200xx_disable(chip); - mutex_destroy(&chip->mutex); -} - static const struct aw200xx_chipdef aw20036_cdef = { .channels = 36, .display_size_rows_max = 3,@@ -652,7 +667,6 @@ static struct i2c_driver aw200xx_driver = { .of_match_table = aw200xx_match_table, }, .probe_new = aw200xx_probe, - .remove = aw200xx_remove, .id_table = aw200xx_id, }; module_i2c_driver(aw200xx_driver);