| Message ID | 1608098978-53068-1-git-send-email-tiantao6@hisilicon.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | power: supply: axp288_charger: switch to using devm_add_action_or_reset() | expand |
Hi, Thank you for your patch, but see comments inline. On 12/16/20 7:09 AM, Tian Tao wrote: > switch to using devm_add_action_or_reset() instead of devm_add_action. This just states what you are changing, not why. If you cannot explain why you are changing something, then possible the change is wrong or at least unnecessary. > > Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > --- > drivers/power/supply/axp288_charger.c | 2 +- > kernel/dma/map_benchmark.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c > index a4df1ea..6480c2e 100644 > --- a/drivers/power/supply/axp288_charger.c > +++ b/drivers/power/supply/axp288_charger.c > @@ -855,7 +855,7 @@ static int axp288_charger_probe(struct platform_device *pdev) > } > > /* Cancel our work on cleanup, register this before the notifiers */ > - ret = devm_add_action(dev, axp288_charger_cancel_work, info); > + ret = devm_add_action_or_reset(dev, axp288_charger_cancel_work, info); As the comment 1 line above the devm_add_action states, the action gets registered *before* the notifiers get registered, so before the work can ever be triggered. IOW there is no need for the reset here. It cannot hurt, but it is not necessay, so NACK. > if (ret) > return ret; > > diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c > index 19f6616..775191d 100644 > --- a/kernel/dma/map_benchmark.c > +++ b/kernel/dma/map_benchmark.c > @@ -294,7 +294,7 @@ static int __map_benchmark_probe(struct device *dev) > return -ENOMEM; > map->dev = dev; > > - ret = devm_add_action(dev, map_benchmark_remove_debugfs, map); > + ret = devm_add_action_or_reset(dev, map_benchmark_remove_debugfs, map); > if (ret) { > pr_err("Can't add debugfs remove action\n"); > return ret; > And this seems to belong in another patch entirely. Regards, Hans
diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c index a4df1ea..6480c2e 100644 --- a/drivers/power/supply/axp288_charger.c +++ b/drivers/power/supply/axp288_charger.c @@ -855,7 +855,7 @@ static int axp288_charger_probe(struct platform_device *pdev) } /* Cancel our work on cleanup, register this before the notifiers */ - ret = devm_add_action(dev, axp288_charger_cancel_work, info); + ret = devm_add_action_or_reset(dev, axp288_charger_cancel_work, info); if (ret) return ret; diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c index 19f6616..775191d 100644 --- a/kernel/dma/map_benchmark.c +++ b/kernel/dma/map_benchmark.c @@ -294,7 +294,7 @@ static int __map_benchmark_probe(struct device *dev) return -ENOMEM; map->dev = dev; - ret = devm_add_action(dev, map_benchmark_remove_debugfs, map); + ret = devm_add_action_or_reset(dev, map_benchmark_remove_debugfs, map); if (ret) { pr_err("Can't add debugfs remove action\n"); return ret;
switch to using devm_add_action_or_reset() instead of devm_add_action. Signed-off-by: Tian Tao <tiantao6@hisilicon.com> --- drivers/power/supply/axp288_charger.c | 2 +- kernel/dma/map_benchmark.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)