| Message ID | 20231204180603.470421-5-gnstark@salutedevices.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | devm_led_classdev_register() usage problem | expand |
On Mon, Dec 4, 2023 at 8:07 PM George Stark <gnstark@salutedevices.com> wrote: > > 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(). ... > +static void aw200xx_chip_reset_action(void *data) > +{ > + const struct aw200xx *chip = (struct aw200xx *)data; > + > + aw200xx_chip_reset(chip); > +} > + > +static void aw200xx_disable_action(void *data) > +{ > + const struct aw200xx *chip = (struct aw200xx *)data; > + > + aw200xx_disable(chip); > +} They can be made oneliners. ... > + if (devm_mutex_init(&client->dev, &chip->mutex)) > + return -ENOMEM; Do not shadow the real error code.
Le 04/12/2023 à 19:05, George Stark a écrit : > 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 <gnstark@salutedevices.com> > --- > 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 > +} > + > 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 ? > > /* 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);
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); +} + +static void aw200xx_disable_action(void *data) +{ + const struct aw200xx *chip = (struct aw200xx *)data; + + aw200xx_disable(chip); +} + 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; /* 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);
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 <gnstark@salutedevices.com> --- drivers/leds/leds-aw200xx.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)