Discussion:
[PATCH 01/13] power: reset: ltc2952: prefer devm_kzalloc over kzalloc
Frans Klaver
2014-10-22 14:30:58 UTC
Permalink
Make use of the fact that the allocated resources can be automatically
deallocated. This reduces cleanup code and chance of leaks.

Signed-off-by: Frans Klaver <***@xsens.com>
---
drivers/power/reset/ltc2952-poweroff.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
index 116a1ce..951a90d 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -292,7 +292,8 @@ static int ltc2952_poweroff_probe(struct platform_device *pdev)
return -EBUSY;
}

- ltc2952_data = kzalloc(sizeof(*ltc2952_data), GFP_KERNEL);
+ ltc2952_data = devm_kzalloc(&pdev->dev, sizeof(*ltc2952_data),
+ GFP_KERNEL);
if (!ltc2952_data)
return -ENOMEM;

@@ -300,17 +301,13 @@ static int ltc2952_poweroff_probe(struct platform_device *pdev)

ret = ltc2952_poweroff_init(pdev);
if (ret)
- goto err;
+ return ret;

pm_power_off = &ltc2952_poweroff_kill;

dev_info(&pdev->dev, "probe successful\n");

return 0;
-
-err:
- kfree(ltc2952_data);
- return ret;
}

static int ltc2952_poweroff_remove(struct platform_device *pdev)
@@ -324,8 +321,6 @@ static int ltc2952_poweroff_remove(struct platform_device *pdev)

for (i = 0; i < ARRAY_SIZE(ltc2952_data->gpio); i++)
gpiod_put(ltc2952_data->gpio[i]);
-
- kfree(ltc2952_data);
}

return 0;
--
2.1.0
Frans Klaver
2014-10-22 14:31:05 UTC
Permalink
ltc2952_poweroff_handler uses gotos to return from the function. Since
we don't do cleanups exiting this function, just return IRQ_HANDLED on
the spot and be done with it.

While at it, remove the variable 'ret'. It was never used very much.

Signed-off-by: Frans Klaver <***@xsens.com>
---
drivers/power/reset/ltc2952-poweroff.c | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
index af3cb6e..70d8e3a 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -153,29 +153,21 @@ ltc2952_poweroff_timer_trigger(struct hrtimer *timer)
*/
static irqreturn_t ltc2952_poweroff_handler(int irq, void *dev_id)
{
- int ret;
struct ltc2952_poweroff *data = dev_id;

- if (data->kernel_panic)
- goto irq_ok;
-
- if (hrtimer_active(&data->timer_wde)) {
+ if (data->kernel_panic || hrtimer_active(&data->timer_wde)) {
/* shutdown is already triggered, nothing to do any more */
- goto irq_ok;
+ return IRQ_HANDLED;
}

if (!hrtimer_active(&data->timer_trigger)) {
- ret = hrtimer_start(&data->timer_trigger, data->trigger_delay,
- HRTIMER_MODE_REL);
-
- if (ret)
+ if (hrtimer_start(&data->timer_trigger, data->trigger_delay,
+ HRTIMER_MODE_REL))
dev_err(data->dev, "unable to start the wait timer\n");
} else {
- ret = hrtimer_cancel(&data->timer_trigger);
+ hrtimer_cancel(&data->timer_trigger);
/* omitting return value check, timer should have been valid */
}
-
-irq_ok:
return IRQ_HANDLED;
}
--
2.1.0
Frans Klaver
2014-10-22 14:31:03 UTC
Permalink
As per Documentation/CodingStyle ch.4, we should keep global variables
to a mininum. Move the panic state into the driver data, regardless of
whether panic is a system state or not.

This removes the need for the custom _init and _exit functions, so
replace them with a call to the module_platform_driver() macro.

Signed-off-by: Frans Klaver <***@xsens.com>
---
drivers/power/reset/ltc2952-poweroff.c | 57 +++++++++++++-------------=
--------
1 file changed, 22 insertions(+), 35 deletions(-)

diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/res=
et/ltc2952-poweroff.c
index 9d493b5..bc8d228 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -75,6 +75,9 @@ struct ltc2952_poweroff {
struct gpio_desc *gpio_trigger;
struct gpio_desc *gpio_watchdog;
struct gpio_desc *gpio_kill;
+
+ bool kernel_panic;
+ struct notifier_block panic_notifier;
};
=20
#define to_ltc2952(p, m) container_of(p, struct ltc2952_poweroff, m)
@@ -84,7 +87,6 @@ struct ltc2952_poweroff {
* remove it entirely once we don't need the global state anymore.
*/
static struct ltc2952_poweroff *ltc2952_data;
-static int ltc2952_poweroff_panic;
=20
/**
* ltc2952_poweroff_timer_wde - Timer callback
@@ -102,7 +104,7 @@ static enum hrtimer_restart ltc2952_poweroff_timer_=
wde(struct hrtimer *timer)
unsigned long overruns;
struct ltc2952_poweroff *data =3D to_ltc2952(timer, timer_wde);
=20
- if (ltc2952_poweroff_panic)
+ if (data->kernel_panic)
return HRTIMER_NORESTART;
=20
state =3D gpiod_get_value(data->gpio_watchdog);
@@ -154,7 +156,7 @@ static irqreturn_t ltc2952_poweroff_handler(int irq=
, void *dev_id)
int ret;
struct ltc2952_poweroff *data =3D dev_id;
=20
- if (ltc2952_poweroff_panic)
+ if (data->kernel_panic)
goto irq_ok;
=20
if (hrtimer_active(&data->timer_wde)) {
@@ -255,6 +257,15 @@ static int ltc2952_poweroff_init(struct platform_d=
evice *pdev)
return 0;
}
=20
+static int ltc2952_poweroff_notify_panic(struct notifier_block *nb,
+ unsigned long code, void *unused)
+{
+ struct ltc2952_poweroff *data =3D to_ltc2952(nb, panic_notifier);
+
+ data->kernel_panic =3D true;
+ return NOTIFY_DONE;
+}
+
static int ltc2952_poweroff_probe(struct platform_device *pdev)
{
int ret;
@@ -280,6 +291,9 @@ static int ltc2952_poweroff_probe(struct platform_d=
evice *pdev)
ltc2952_data =3D data;
pm_power_off =3D &ltc2952_poweroff_kill;
=20
+ data->panic_notifier.notifier_call =3D ltc2952_poweroff_notify_panic;
+ atomic_notifier_chain_register(&panic_notifier_list,
+ &data->panic_notifier);
dev_info(&pdev->dev, "probe successful\n");
=20
return 0;
@@ -287,8 +301,11 @@ static int ltc2952_poweroff_probe(struct platform_=
device *pdev)
=20
static int ltc2952_poweroff_remove(struct platform_device *pdev)
{
- pm_power_off =3D NULL;
+ struct ltc2952_poweroff *data =3D platform_get_drvdata(pdev);
=20
+ pm_power_off =3D NULL;
+ atomic_notifier_chain_unregister(&panic_notifier_list,
+ &data->panic_notifier);
return 0;
}
=20
@@ -310,37 +327,7 @@ static struct platform_driver ltc2952_poweroff_dri=
ver =3D {
.resume =3D ltc2952_poweroff_resume,
};
=20
-static int ltc2952_poweroff_notify_panic(struct notifier_block *nb,
- unsigned long code, void *unused)
-{
- ltc2952_poweroff_panic =3D 1;
- return NOTIFY_DONE;
-}
-
-static struct notifier_block ltc2952_poweroff_panic_nb =3D {
- .notifier_call =3D ltc2952_poweroff_notify_panic,
-};
-
-static int __init ltc2952_poweroff_platform_init(void)
-{
- ltc2952_poweroff_panic =3D 0;
-
- atomic_notifier_chain_register(&panic_notifier_list,
- &ltc2952_poweroff_panic_nb);
-
- return platform_driver_register(&ltc2952_poweroff_driver);
-}
-
-static void __exit ltc2952_poweroff_platform_exit(void)
-{
- atomic_notifier_chain_unregister(&panic_notifier_list,
- &ltc2952_poweroff_panic_nb);
-
- platform_driver_unregister(&ltc2952_poweroff_driver);
-}
-
-module_init(ltc2952_poweroff_platform_init);
-module_exit(ltc2952_poweroff_platform_exit);
+module_platform_driver(ltc2952_poweroff_driver);
=20
MODULE_AUTHOR("Ren=E9 Moll <***@xsens.com>");
MODULE_DESCRIPTION("LTC PowerPath power-off driver");
--=20
2.1.0
Frans Klaver
2014-10-22 14:31:01 UTC
Permalink
This reduces cleanup code and chance of errors.

Signed-off-by: Frans Klaver <***@xsens.com>
---
drivers/power/reset/ltc2952-poweroff.c | 44 ++++++++--------------------------
1 file changed, 10 insertions(+), 34 deletions(-)

diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
index fc583e0..1e96b5b 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -207,44 +207,34 @@ static int ltc2952_poweroff_init(struct platform_device *pdev)
data = ltc2952_data;
ltc2952_poweroff_default(ltc2952_data);

- ltc2952_data->gpio_watchdog = gpiod_get(&pdev->dev, "watchdog");
+ ltc2952_data->gpio_watchdog = devm_gpiod_get(&pdev->dev, "watchdog",
+ GPIOD_OUT_LOW);
if (IS_ERR(ltc2952_data->gpio_watchdog)) {
ret = PTR_ERR(ltc2952_data->gpio_watchdog);
dev_err(&pdev->dev, "unable to claim gpio \"watchdog\"\n");
return ret;
}

- ltc2952_data->gpio_kill = gpiod_get(&pdev->dev, "kill");
+ ltc2952_data->gpio_kill = devm_gpiod_get(&pdev->dev, "kill",
+ GPIOD_OUT_LOW);
if (IS_ERR(ltc2952_data->gpio_kill)) {
ret = PTR_ERR(ltc2952_data->gpio_kill);
dev_err(&pdev->dev, "unable to claim gpio \"kill\"\n");
- goto err_kill;
+ return ret;
}

- ltc2952_data->gpio_trigger = gpiod_get(&pdev->dev, "trigger");
+ ltc2952_data->gpio_trigger = devm_gpiod_get(&pdev->dev, "trigger",
+ GPIOD_IN);
if (IS_ERR(ltc2952_data->gpio_trigger)) {
ret = PTR_ERR(ltc2952_data->gpio_trigger);
dev_err(&pdev->dev, "unable to claim gpio \"trigger\"\n");
- goto err_trigger;
- }
-
- ret = gpiod_direction_output(
- ltc2952_data->gpio_watchdog, 0);
- if (ret) {
- dev_err(&pdev->dev, "unable to use watchdog-gpio as output\n");
- goto err_io;
- }
-
- ret = gpiod_direction_output(ltc2952_data->gpio_kill, 0);
- if (ret) {
- dev_err(&pdev->dev, "unable to use kill-gpio as output\n");
- goto err_io;
+ return ret;
}

virq = gpiod_to_irq(ltc2952_data->gpio_trigger);
if (virq < 0) {
dev_err(&pdev->dev, "cannot map GPIO as interrupt");
- goto err_io;
+ return ret;
}

ret = devm_request_irq(&pdev->dev, virq,
@@ -255,19 +245,10 @@ static int ltc2952_poweroff_init(struct platform_device *pdev)

if (ret) {
dev_err(&pdev->dev, "cannot configure an interrupt handler\n");
- goto err_io;
+ return ret;
}

return 0;
-
-err_io:
- gpiod_put(ltc2952_data->gpio_trigger);
-err_trigger:
- gpiod_put(ltc2952_data->gpio_kill);
-err_kill:
- gpiod_put(ltc2952_data->gpio_watchdog);
-
- return ret;
}

static int ltc2952_poweroff_probe(struct platform_device *pdev)
@@ -301,11 +282,6 @@ static int ltc2952_poweroff_remove(struct platform_device *pdev)
{
pm_power_off = NULL;

- if (ltc2952_data) {
- gpiod_put(ltc2952_data->gpio_trigger);
- gpiod_put(ltc2952_data->gpio_watchdog);
- gpiod_put(ltc2952_data->gpio_kill);
- }
return 0;
}
--
2.1.0
Frans Klaver
2014-10-22 14:31:07 UTC
Permalink
Disable the timers when ltc2952_poweroff is removed. We don't want to
risk calling functions on data that no longer exist.

Signed-off-by: Frans Klaver <***@xsens.com>
---
drivers/power/reset/ltc2952-poweroff.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
index 8ab1da6..2f3fd5b 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -285,6 +285,8 @@ static int ltc2952_poweroff_remove(struct platform_device *pdev)
struct ltc2952_poweroff *data = platform_get_drvdata(pdev);

pm_power_off = NULL;
+ hrtimer_cancel(&data->timer_trigger);
+ hrtimer_cancel(&data->timer_wde);
atomic_notifier_chain_unregister(&panic_notifier_list,
&data->panic_notifier);
return 0;
--
2.1.0
Frans Klaver
2014-10-22 14:31:08 UTC
Permalink
In ltc2952_poweroff_handler it is theoretically possible that the timer
fails to start on first pass (button press), but succeeds in starting on
the second (button release). This will cause the button press to be
misinterpreted, and will incorrectly shut down the system. Because a
picture says more than a thousand words:

Expected behavior:
tmr: ++++++++++
btn: -----__________-----

Faulty behavior:
tmr: +++++
btn: -----__________-----

Legend:
+ timer runs
_ button pressed
- button depressed

To prevent this from happening, check the value of the gpio before
starting the timer. If the button is active, we should start the timer,
else we should stop it.

The situation described can now still occur if the polarity of the input
pin is set incorrectly, but that at least is predictable behavior and
can be detected during the first tests.

Signed-off-by: Frans Klaver <***@xsens.com>
---
drivers/power/reset/ltc2952-poweroff.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
index 2f3fd5b..fda2735 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -160,7 +160,7 @@ static irqreturn_t ltc2952_poweroff_handler(int irq, void *dev_id)
return IRQ_HANDLED;
}

- if (!hrtimer_active(&data->timer_trigger)) {
+ if (gpiod_get_value(data->gpio_trigger)) {
if (hrtimer_start(&data->timer_trigger, data->trigger_delay,
HRTIMER_MODE_REL))
dev_err(data->dev, "unable to start the wait timer\n");
--
2.1.0
Frans Klaver
2014-10-22 14:31:10 UTC
Permalink
Document the fact that the trigger signal is now optional, and describe
the behavior when this is used.

While at it, fix a typo, and paraphrase a sentence to be less platform
specific.

Signed-off-by: Frans Klaver <***@xsens.com>
---
.../devicetree/bindings/power/reset/ltc2952-poweroff.txt | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/reset/ltc2952-poweroff.txt b/Documentation/devicetree/bindings/power/reset/ltc2952-poweroff.txt
index 0c94c63..cd2d7f5 100644
--- a/Documentation/devicetree/bindings/power/reset/ltc2952-poweroff.txt
+++ b/Documentation/devicetree/bindings/power/reset/ltc2952-poweroff.txt
@@ -1,20 +1,23 @@
Binding for the LTC2952 PowerPath controller

This chip is used to externally trigger a system shut down. Once the trigger has
-been sent, the chips' watchdog has to be reset to gracefully shut down.
-If the Linux systems decides to shut down it powers off the platform via the
-kill signal.
+been sent, the chip's watchdog has to be reset to gracefully shut down.
+A full powerdown can be triggered via the kill signal.

Required properties:

- compatible: Must contain: "lltc,ltc2952"
-- trigger-gpios: phandle + gpio-specifier for the GPIO connected to the
- chip's trigger line
- watchdog-gpios: phandle + gpio-specifier for the GPIO connected to the
chip's watchdog line
- kill-gpios: phandle + gpio-specifier for the GPIO connected to the
chip's kill line

+Optional properties:
+- trigger-gpios: phandle + gpio-specifier for the GPIO connected to the
+ chip's trigger line. If this property is not set, the
+ trigger function is ignored and the chip is kept alive
+ until an explicit kill signal is received
+
Example:

ltc2952 {
--
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Frans Klaver
2014-10-22 14:31:09 UTC
Permalink
Currently the ltc2952 supports only one button sequence to initiate
powerdown. This is not always desirable, as even prolonged button
presses can happen in use.

Allow ltc2952 users to pick their own power down sequence, by making the
trigger input optional. Since this still means that the ltc2952 may
power down the platform if the power button is pressed for about 5
seconds, we still need to make sure to start the watchdog toggle to
prolong the system power for as long as we need it.

This will still allow the system to control power using the kill signal.

Signed-off-by: Frans Klaver <***@xsens.com>
---
drivers/power/reset/ltc2952-poweroff.c | 87 +++++++++++++++++++++-------------
1 file changed, 55 insertions(+), 32 deletions(-)

diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
index fda2735..72e3d7f 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -32,7 +32,9 @@
* - trigger (input)
* A level change indicates the shut-down trigger. If it's state reverts
* within the time-out defined by trigger_delay, the shut down is not
- * executed.
+ * executed. If no pin is assigned to this input, the driver will start the
+ * watchdog toggle immediately. The chip will only power off the system if
+ * it is requested to do so through the kill line.
*
* - watchdog (output)
* Once a shut down is triggered, the driver will toggle this signal,
@@ -116,15 +118,10 @@ static enum hrtimer_restart ltc2952_poweroff_timer_wde(struct hrtimer *timer)
return HRTIMER_RESTART;
}

-static enum hrtimer_restart
-ltc2952_poweroff_timer_trigger(struct hrtimer *timer)
+static void ltc2952_poweroff_start_wde(struct ltc2952_poweroff *data)
{
- struct ltc2952_poweroff *data = to_ltc2952(timer, timer_trigger);
- int ret = hrtimer_start(&data->timer_wde,
- data->wde_interval, HRTIMER_MODE_REL);
-
- if (ret) {
- dev_err(data->dev, "unable to start the timer\n");
+ if (hrtimer_start(&data->timer_wde, data->wde_interval,
+ HRTIMER_MODE_REL)) {
/*
* The device will not toggle the watchdog reset,
* thus shut down is only safe if the PowerPath controller
@@ -133,10 +130,17 @@ ltc2952_poweroff_timer_trigger(struct hrtimer *timer)
*
* Only sending a warning as the system will power-off anyway
*/
+ dev_err(data->dev, "unable to start the timer\n");
}
+}

- dev_info(data->dev, "executing shutdown\n");
+static enum hrtimer_restart
+ltc2952_poweroff_timer_trigger(struct hrtimer *timer)
+{
+ struct ltc2952_poweroff *data = to_ltc2952(timer, timer_trigger);

+ ltc2952_poweroff_start_wde(data);
+ dev_info(data->dev, "executing shutdown\n");
orderly_poweroff(true);

return HRTIMER_NORESTART;
@@ -190,7 +194,7 @@ static void ltc2952_poweroff_default(struct ltc2952_poweroff *data)

static int ltc2952_poweroff_init(struct platform_device *pdev)
{
- int ret, virq;
+ int ret;
struct ltc2952_poweroff *data = platform_get_drvdata(pdev);

ltc2952_poweroff_default(data);
@@ -210,29 +214,48 @@ static int ltc2952_poweroff_init(struct platform_device *pdev)
return ret;
}

- data->gpio_trigger = devm_gpiod_get(&pdev->dev, "trigger",
- GPIOD_IN);
- if (IS_ERR(ltc2952_data->gpio_trigger)) {
- ret = PTR_ERR(ltc2952_data->gpio_trigger);
- dev_err(&pdev->dev, "unable to claim gpio \"trigger\"\n");
- return ret;
- }
-
- virq = gpiod_to_irq(data->gpio_trigger);
- if (virq < 0) {
- dev_err(&pdev->dev, "cannot map GPIO as interrupt");
- return ret;
+ data->gpio_trigger = devm_gpiod_get(&pdev->dev, "trigger", GPIOD_IN);
+ if (IS_ERR(data->gpio_trigger)) {
+ /*
+ * It's not a problem if the trigger gpio isn't available, but
+ * it is worth a warning if its use was defined in the device
+ * tree.
+ */
+ if (PTR_ERR(data->gpio_trigger) != -ENOENT)
+ dev_err(&pdev->dev,
+ "unable to claim gpio \"trigger\"\n");
+ data->gpio_trigger = NULL;
}

- ret = devm_request_irq(&pdev->dev, virq,
- ltc2952_poweroff_handler,
- (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING),
- "ltc2952-poweroff",
- data);
-
- if (ret) {
- dev_err(&pdev->dev, "cannot configure an interrupt handler\n");
- return ret;
+ if (devm_request_irq(&pdev->dev, gpiod_to_irq(data->gpio_trigger),
+ ltc2952_poweroff_handler,
+ (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING),
+ "ltc2952-poweroff",
+ data)) {
+ /*
+ * Some things may have happened:
+ * - No trigger input was defined
+ * - Claiming the GPIO failed
+ * - We could not map to an IRQ
+ * - We couldn't register an interrupt handler
+ *
+ * None of these really are problems, but all of them
+ * disqualify the push button from controlling the power.
+ *
+ * It is therefore important to note that if the ltc2952
+ * detects a button press for long enough, it will still start
+ * its own powerdown window and cut the power on us if we don't
+ * start the watchdog trigger.
+ */
+ if (data->gpio_trigger) {
+ dev_warn(&pdev->dev,
+ "unable to configure the trigger interrupt\n");
+ devm_gpiod_put(&pdev->dev, data->gpio_trigger);
+ data->gpio_trigger = NULL;
+ }
+ dev_info(&pdev->dev,
+ "power down trigger input will not be used\n");
+ ltc2952_poweroff_start_wde(data);
}

return 0;
--
2.1.0
Frans Klaver
2014-10-23 08:44:05 UTC
Permalink
Post by Frans Klaver
Currently the ltc2952 supports only one button sequence to initiate
powerdown. This is not always desirable, as even prolonged button
presses can happen in use.
Allow ltc2952 users to pick their own power down sequence, by making the
trigger input optional. Since this still means that the ltc2952 may
power down the platform if the power button is pressed for about 5
seconds, we still need to make sure to start the watchdog toggle to
prolong the system power for as long as we need it.
This will still allow the system to control power using the kill signal.
---
drivers/power/reset/ltc2952-poweroff.c | 87 +++++++++++++++++++++-------------
1 file changed, 55 insertions(+), 32 deletions(-)
diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
index fda2735..72e3d7f 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -32,7 +32,9 @@
* - trigger (input)
* A level change indicates the shut-down trigger. If it's state reverts
* within the time-out defined by trigger_delay, the shut down is not
- * executed.
+ * executed. If no pin is assigned to this input, the driver will start the
+ * watchdog toggle immediately. The chip will only power off the system if
+ * it is requested to do so through the kill line.
*
* - watchdog (output)
* Once a shut down is triggered, the driver will toggle this signal,
@@ -116,15 +118,10 @@ static enum hrtimer_restart ltc2952_poweroff_timer_wde(struct hrtimer *timer)
return HRTIMER_RESTART;
}
-static enum hrtimer_restart
-ltc2952_poweroff_timer_trigger(struct hrtimer *timer)
+static void ltc2952_poweroff_start_wde(struct ltc2952_poweroff *data)
{
- struct ltc2952_poweroff *data = to_ltc2952(timer, timer_trigger);
- int ret = hrtimer_start(&data->timer_wde,
- data->wde_interval, HRTIMER_MODE_REL);
-
- if (ret) {
- dev_err(data->dev, "unable to start the timer\n");
+ if (hrtimer_start(&data->timer_wde, data->wde_interval,
+ HRTIMER_MODE_REL)) {
/*
* The device will not toggle the watchdog reset,
* thus shut down is only safe if the PowerPath controller
@@ -133,10 +130,17 @@ ltc2952_poweroff_timer_trigger(struct hrtimer *timer)
*
* Only sending a warning as the system will power-off anyway
*/
+ dev_err(data->dev, "unable to start the timer\n");
}
+}
- dev_info(data->dev, "executing shutdown\n");
+static enum hrtimer_restart
+ltc2952_poweroff_timer_trigger(struct hrtimer *timer)
+{
+ struct ltc2952_poweroff *data = to_ltc2952(timer, timer_trigger);
+ ltc2952_poweroff_start_wde(data);
+ dev_info(data->dev, "executing shutdown\n");
orderly_poweroff(true);
return HRTIMER_NORESTART;
@@ -190,7 +194,7 @@ static void ltc2952_poweroff_default(struct ltc2952_poweroff *data)
static int ltc2952_poweroff_init(struct platform_device *pdev)
{
- int ret, virq;
+ int ret;
struct ltc2952_poweroff *data = platform_get_drvdata(pdev);
ltc2952_poweroff_default(data);
@@ -210,29 +214,48 @@ static int ltc2952_poweroff_init(struct platform_device *pdev)
return ret;
}
- data->gpio_trigger = devm_gpiod_get(&pdev->dev, "trigger",
- GPIOD_IN);
- if (IS_ERR(ltc2952_data->gpio_trigger)) {
- ret = PTR_ERR(ltc2952_data->gpio_trigger);
- dev_err(&pdev->dev, "unable to claim gpio \"trigger\"\n");
- return ret;
- }
-
- virq = gpiod_to_irq(data->gpio_trigger);
- if (virq < 0) {
- dev_err(&pdev->dev, "cannot map GPIO as interrupt");
- return ret;
+ data->gpio_trigger = devm_gpiod_get(&pdev->dev, "trigger", GPIOD_IN);
+ if (IS_ERR(data->gpio_trigger)) {
+ /*
+ * It's not a problem if the trigger gpio isn't available, but
+ * it is worth a warning if its use was defined in the device
+ * tree.
+ */
+ if (PTR_ERR(data->gpio_trigger) != -ENOENT)
+ dev_err(&pdev->dev,
+ "unable to claim gpio \"trigger\"\n");
+ data->gpio_trigger = NULL;
}
I just realized devm_gpiod_get_optional() may be slightly cleaner here,
but the difference isn't that big in the end:

+ data->gpio_trigger = devm_gpiod_get_optional(&pdev->dev, "trigger",
+ GPIOD_IN);
+ if (IS_ERR(data->gpio_trigger)) {
+ /*
+ * It's not a problem if the trigger gpio isn't available, but
+ * it is worth a warning if its use was defined in the device
+ * tree.
+ */
+ dev_err(&pdev->dev, "unable to claim gpio \"trigger\"\n");
+ data->gpio_trigger = NULL;
}
Post by Frans Klaver
- ret = devm_request_irq(&pdev->dev, virq,
- ltc2952_poweroff_handler,
- (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING),
- "ltc2952-poweroff",
- data);
-
- if (ret) {
- dev_err(&pdev->dev, "cannot configure an interrupt handler\n");
- return ret;
+ if (devm_request_irq(&pdev->dev, gpiod_to_irq(data->gpio_trigger),
+ ltc2952_poweroff_handler,
+ (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING),
+ "ltc2952-poweroff",
+ data)) {
+ /*
+ * - No trigger input was defined
+ * - Claiming the GPIO failed
+ * - We could not map to an IRQ
+ * - We couldn't register an interrupt handler
+ *
+ * None of these really are problems, but all of them
+ * disqualify the push button from controlling the power.
+ *
+ * It is therefore important to note that if the ltc2952
+ * detects a button press for long enough, it will still start
+ * its own powerdown window and cut the power on us if we don't
+ * start the watchdog trigger.
+ */
+ if (data->gpio_trigger) {
+ dev_warn(&pdev->dev,
+ "unable to configure the trigger interrupt\n");
+ devm_gpiod_put(&pdev->dev, data->gpio_trigger);
+ data->gpio_trigger = NULL;
+ }
+ dev_info(&pdev->dev,
+ "power down trigger input will not be used\n");
+ ltc2952_poweroff_start_wde(data);
}
return 0;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Frans Klaver
2014-10-22 14:31:06 UTC
Permalink
The function pointers for the timers and pm_power_off are assigned with
C++ style
foo = &func;

Let's change it instead to the more C style
foo = func;

Signed-off-by: Frans Klaver <***@xsens.com>
---
drivers/power/reset/ltc2952-poweroff.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
index 70d8e3a..8ab1da6 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -182,10 +182,10 @@ static void ltc2952_poweroff_default(struct ltc2952_poweroff *data)
data->trigger_delay = ktime_set(2, 500L*1E6L);

hrtimer_init(&data->timer_trigger, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
- data->timer_trigger.function = &ltc2952_poweroff_timer_trigger;
+ data->timer_trigger.function = ltc2952_poweroff_timer_trigger;

hrtimer_init(&data->timer_wde, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
- data->timer_wde.function = &ltc2952_poweroff_timer_wde;
+ data->timer_wde.function = ltc2952_poweroff_timer_wde;
}

static int ltc2952_poweroff_init(struct platform_device *pdev)
@@ -270,7 +270,7 @@ static int ltc2952_poweroff_probe(struct platform_device *pdev)

/* TODO: remove ltc2952_data */
ltc2952_data = data;
- pm_power_off = &ltc2952_poweroff_kill;
+ pm_power_off = ltc2952_poweroff_kill;

data->panic_notifier.notifier_call = ltc2952_poweroff_notify_panic;
atomic_notifier_chain_register(&panic_notifier_list,
--
2.1.0
Frans Klaver
2014-10-22 14:31:04 UTC
Permalink
Documentation/SubmittingDrivers suggests these be implemented even when
they do nothing. On the other hand, the platform code calls these
functions 'legacy'. Suspend and resume operations should go into a
pm_ops structure, pointed at by the driver's pm field. This approach
would lead to a lot of boiler plate, while achieving nothing. Drop the
functions instead.

Signed-off-by: Frans Klaver <***@xsens.com>
---
drivers/power/reset/ltc2952-poweroff.c | 13 -------------
1 file changed, 13 deletions(-)

diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
index bc8d228..af3cb6e 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -184,17 +184,6 @@ static void ltc2952_poweroff_kill(void)
gpiod_set_value(ltc2952_data->gpio_kill, 1);
}

-static int ltc2952_poweroff_suspend(struct platform_device *pdev,
- pm_message_t state)
-{
- return -ENOSYS;
-}
-
-static int ltc2952_poweroff_resume(struct platform_device *pdev)
-{
- return -ENOSYS;
-}
-
static void ltc2952_poweroff_default(struct ltc2952_poweroff *data)
{
data->wde_interval = ktime_set(0, 300L*1E6L);
@@ -323,8 +312,6 @@ static struct platform_driver ltc2952_poweroff_driver = {
.owner = THIS_MODULE,
.of_match_table = of_ltc2952_poweroff_match,
},
- .suspend = ltc2952_poweroff_suspend,
- .resume = ltc2952_poweroff_resume,
};

module_platform_driver(ltc2952_poweroff_driver);
--
2.1.0
Frans Klaver
2014-10-22 14:31:02 UTC
Permalink
Documentation/CodingStyle ch.4 mentions in a side node that global
variables should only be used if you really need them. Reduce the use of
the global instance of ltc2952_poweroff so we may eventually remove it
entirely.

While at it, rename ltc2952_poweroff_data to ltc2952_poweroff, just to
save that little bit of typing.

Signed-off-by: Frans Klaver <***@xsens.com>
---
drivers/power/reset/ltc2952-poweroff.c | 75 +++++++++++++++++++---------------
1 file changed, 41 insertions(+), 34 deletions(-)

diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
index 1e96b5b..9d493b5 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -63,7 +63,7 @@
#include <linux/gpio/consumer.h>
#include <linux/reboot.h>

-struct ltc2952_poweroff_data {
+struct ltc2952_poweroff {
struct hrtimer timer_trigger;
struct hrtimer timer_wde;

@@ -77,8 +77,14 @@ struct ltc2952_poweroff_data {
struct gpio_desc *gpio_kill;
};

+#define to_ltc2952(p, m) container_of(p, struct ltc2952_poweroff, m)
+
+/*
+ * This global variable is only needed for pm_power_off. We should
+ * remove it entirely once we don't need the global state anymore.
+ */
+static struct ltc2952_poweroff *ltc2952_data;
static int ltc2952_poweroff_panic;
-static struct ltc2952_poweroff_data *ltc2952_data;

/**
* ltc2952_poweroff_timer_wde - Timer callback
@@ -94,29 +100,29 @@ static enum hrtimer_restart ltc2952_poweroff_timer_wde(struct hrtimer *timer)
ktime_t now;
int state;
unsigned long overruns;
+ struct ltc2952_poweroff *data = to_ltc2952(timer, timer_wde);

if (ltc2952_poweroff_panic)
return HRTIMER_NORESTART;

- state = gpiod_get_value(ltc2952_data->gpio_watchdog);
- gpiod_set_value(ltc2952_data->gpio_watchdog, !state);
+ state = gpiod_get_value(data->gpio_watchdog);
+ gpiod_set_value(data->gpio_watchdog, !state);

now = hrtimer_cb_get_time(timer);
- overruns = hrtimer_forward(timer, now, ltc2952_data->wde_interval);
+ overruns = hrtimer_forward(timer, now, data->wde_interval);

return HRTIMER_RESTART;
}

-static enum hrtimer_restart ltc2952_poweroff_timer_trigger(
- struct hrtimer *timer)
+static enum hrtimer_restart
+ltc2952_poweroff_timer_trigger(struct hrtimer *timer)
{
- int ret;
-
- ret = hrtimer_start(&ltc2952_data->timer_wde,
- ltc2952_data->wde_interval, HRTIMER_MODE_REL);
+ struct ltc2952_poweroff *data = to_ltc2952(timer, timer_trigger);
+ int ret = hrtimer_start(&data->timer_wde,
+ data->wde_interval, HRTIMER_MODE_REL);

if (ret) {
- dev_err(ltc2952_data->dev, "unable to start the timer\n");
+ dev_err(data->dev, "unable to start the timer\n");
/*
* The device will not toggle the watchdog reset,
* thus shut down is only safe if the PowerPath controller
@@ -127,7 +133,7 @@ static enum hrtimer_restart ltc2952_poweroff_timer_trigger(
*/
}

- dev_info(ltc2952_data->dev, "executing shutdown\n");
+ dev_info(data->dev, "executing shutdown\n");

orderly_poweroff(true);

@@ -146,7 +152,7 @@ static enum hrtimer_restart ltc2952_poweroff_timer_trigger(
static irqreturn_t ltc2952_poweroff_handler(int irq, void *dev_id)
{
int ret;
- struct ltc2952_poweroff_data *data = dev_id;
+ struct ltc2952_poweroff *data = dev_id;

if (ltc2952_poweroff_panic)
goto irq_ok;
@@ -187,7 +193,7 @@ static int ltc2952_poweroff_resume(struct platform_device *pdev)
return -ENOSYS;
}

-static void ltc2952_poweroff_default(struct ltc2952_poweroff_data *data)
+static void ltc2952_poweroff_default(struct ltc2952_poweroff *data)
{
data->wde_interval = ktime_set(0, 300L*1E6L);
data->trigger_delay = ktime_set(2, 500L*1E6L);
@@ -202,36 +208,34 @@ static void ltc2952_poweroff_default(struct ltc2952_poweroff_data *data)
static int ltc2952_poweroff_init(struct platform_device *pdev)
{
int ret, virq;
- struct ltc2952_poweroff_data *data;
+ struct ltc2952_poweroff *data = platform_get_drvdata(pdev);

- data = ltc2952_data;
- ltc2952_poweroff_default(ltc2952_data);
+ ltc2952_poweroff_default(data);

- ltc2952_data->gpio_watchdog = devm_gpiod_get(&pdev->dev, "watchdog",
- GPIOD_OUT_LOW);
- if (IS_ERR(ltc2952_data->gpio_watchdog)) {
- ret = PTR_ERR(ltc2952_data->gpio_watchdog);
+ data->gpio_watchdog = devm_gpiod_get(&pdev->dev, "watchdog",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(data->gpio_watchdog)) {
+ ret = PTR_ERR(data->gpio_watchdog);
dev_err(&pdev->dev, "unable to claim gpio \"watchdog\"\n");
return ret;
}

- ltc2952_data->gpio_kill = devm_gpiod_get(&pdev->dev, "kill",
- GPIOD_OUT_LOW);
- if (IS_ERR(ltc2952_data->gpio_kill)) {
- ret = PTR_ERR(ltc2952_data->gpio_kill);
+ data->gpio_kill = devm_gpiod_get(&pdev->dev, "kill", GPIOD_OUT_LOW);
+ if (IS_ERR(data->gpio_kill)) {
+ ret = PTR_ERR(data->gpio_kill);
dev_err(&pdev->dev, "unable to claim gpio \"kill\"\n");
return ret;
}

- ltc2952_data->gpio_trigger = devm_gpiod_get(&pdev->dev, "trigger",
- GPIOD_IN);
+ data->gpio_trigger = devm_gpiod_get(&pdev->dev, "trigger",
+ GPIOD_IN);
if (IS_ERR(ltc2952_data->gpio_trigger)) {
ret = PTR_ERR(ltc2952_data->gpio_trigger);
dev_err(&pdev->dev, "unable to claim gpio \"trigger\"\n");
return ret;
}

- virq = gpiod_to_irq(ltc2952_data->gpio_trigger);
+ virq = gpiod_to_irq(data->gpio_trigger);
if (virq < 0) {
dev_err(&pdev->dev, "cannot map GPIO as interrupt");
return ret;
@@ -241,7 +245,7 @@ static int ltc2952_poweroff_init(struct platform_device *pdev)
ltc2952_poweroff_handler,
(IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING),
"ltc2952-poweroff",
- ltc2952_data);
+ data);

if (ret) {
dev_err(&pdev->dev, "cannot configure an interrupt handler\n");
@@ -254,23 +258,26 @@ static int ltc2952_poweroff_init(struct platform_device *pdev)
static int ltc2952_poweroff_probe(struct platform_device *pdev)
{
int ret;
+ struct ltc2952_poweroff *data;

if (pm_power_off) {
dev_err(&pdev->dev, "pm_power_off already registered");
return -EBUSY;
}

- ltc2952_data = devm_kzalloc(&pdev->dev, sizeof(*ltc2952_data),
- GFP_KERNEL);
- if (!ltc2952_data)
+ data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
return -ENOMEM;

- ltc2952_data->dev = &pdev->dev;
+ data->dev = &pdev->dev;
+ platform_set_drvdata(pdev, data);

ret = ltc2952_poweroff_init(pdev);
if (ret)
return ret;

+ /* TODO: remove ltc2952_data */
+ ltc2952_data = data;
pm_power_off = &ltc2952_poweroff_kill;

dev_info(&pdev->dev, "probe successful\n");
--
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Frans Klaver
2014-10-22 14:31:00 UTC
Permalink
The three gpio's used by this driver are stored in an array of pointers.
This doesn't add much besides cleanups in a loop. In fact, it makes most
of the usage sites harder to read. Unroll the loop, and live with the
fact that cleanups become slightly larger.

Signed-off-by: Frans Klaver <***@xsens.com>
---
drivers/power/reset/ltc2952-poweroff.c | 86 +++++++++++++++-------------------
1 file changed, 38 insertions(+), 48 deletions(-)

diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
index 2c11316..fc583e0 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -72,21 +72,14 @@ struct ltc2952_poweroff_data {

struct device *dev;

- /**
- * 0: trigger
- * 1: watchdog
- * 2: kill
- */
- struct gpio_desc *gpio[3];
+ struct gpio_desc *gpio_trigger;
+ struct gpio_desc *gpio_watchdog;
+ struct gpio_desc *gpio_kill;
};

static int ltc2952_poweroff_panic;
static struct ltc2952_poweroff_data *ltc2952_data;

-#define POWERPATH_IO_TRIGGER 0
-#define POWERPATH_IO_WATCHDOG 1
-#define POWERPATH_IO_KILL 2
-
/**
* ltc2952_poweroff_timer_wde - Timer callback
* Toggles the watchdog reset signal each wde_interval
@@ -105,8 +98,8 @@ static enum hrtimer_restart ltc2952_poweroff_timer_wde(struct hrtimer *timer)
if (ltc2952_poweroff_panic)
return HRTIMER_NORESTART;

- state = gpiod_get_value(ltc2952_data->gpio[POWERPATH_IO_WATCHDOG]);
- gpiod_set_value(ltc2952_data->gpio[POWERPATH_IO_WATCHDOG], !state);
+ state = gpiod_get_value(ltc2952_data->gpio_watchdog);
+ gpiod_set_value(ltc2952_data->gpio_watchdog, !state);

now = hrtimer_cb_get_time(timer);
overruns = hrtimer_forward(timer, now, ltc2952_data->wde_interval);
@@ -120,7 +113,7 @@ static enum hrtimer_restart ltc2952_poweroff_timer_trigger(
int ret;

ret = hrtimer_start(&ltc2952_data->timer_wde,
- ltc2952_data->wde_interval, HRTIMER_MODE_REL);
+ ltc2952_data->wde_interval, HRTIMER_MODE_REL);

if (ret) {
dev_err(ltc2952_data->dev, "unable to start the timer\n");
@@ -180,7 +173,7 @@ irq_ok:

static void ltc2952_poweroff_kill(void)
{
- gpiod_set_value(ltc2952_data->gpio[POWERPATH_IO_KILL], 1);
+ gpiod_set_value(ltc2952_data->gpio_kill, 1);
}

static int ltc2952_poweroff_suspend(struct platform_device *pdev,
@@ -196,11 +189,6 @@ static int ltc2952_poweroff_resume(struct platform_device *pdev)

static void ltc2952_poweroff_default(struct ltc2952_poweroff_data *data)
{
- unsigned int i;
-
- for (i = 0; i < ARRAY_SIZE(data->gpio); i++)
- data->gpio[i] = NULL;
-
data->wde_interval = ktime_set(0, 300L*1E6L);
data->trigger_delay = ktime_set(2, 500L*1E6L);

@@ -214,45 +202,46 @@ static void ltc2952_poweroff_default(struct ltc2952_poweroff_data *data)
static int ltc2952_poweroff_init(struct platform_device *pdev)
{
int ret, virq;
- unsigned int i;
struct ltc2952_poweroff_data *data;

- static char *name[] = {
- "trigger",
- "watchdog",
- "kill",
- NULL
- };
-
data = ltc2952_data;
ltc2952_poweroff_default(ltc2952_data);

- for (i = 0; i < ARRAY_SIZE(ltc2952_data->gpio); i++) {
- ltc2952_data->gpio[i] = gpiod_get(&pdev->dev, name[i]);
+ ltc2952_data->gpio_watchdog = gpiod_get(&pdev->dev, "watchdog");
+ if (IS_ERR(ltc2952_data->gpio_watchdog)) {
+ ret = PTR_ERR(ltc2952_data->gpio_watchdog);
+ dev_err(&pdev->dev, "unable to claim gpio \"watchdog\"\n");
+ return ret;
+ }

- if (IS_ERR(ltc2952_data->gpio[i])) {
- ret = PTR_ERR(ltc2952_data->gpio[i]);
- dev_err(&pdev->dev,
- "unable to claim the following gpio: %s\n",
- name[i]);
- goto err_io;
- }
+ ltc2952_data->gpio_kill = gpiod_get(&pdev->dev, "kill");
+ if (IS_ERR(ltc2952_data->gpio_kill)) {
+ ret = PTR_ERR(ltc2952_data->gpio_kill);
+ dev_err(&pdev->dev, "unable to claim gpio \"kill\"\n");
+ goto err_kill;
+ }
+
+ ltc2952_data->gpio_trigger = gpiod_get(&pdev->dev, "trigger");
+ if (IS_ERR(ltc2952_data->gpio_trigger)) {
+ ret = PTR_ERR(ltc2952_data->gpio_trigger);
+ dev_err(&pdev->dev, "unable to claim gpio \"trigger\"\n");
+ goto err_trigger;
}

ret = gpiod_direction_output(
- ltc2952_data->gpio[POWERPATH_IO_WATCHDOG], 0);
+ ltc2952_data->gpio_watchdog, 0);
if (ret) {
dev_err(&pdev->dev, "unable to use watchdog-gpio as output\n");
goto err_io;
}

- ret = gpiod_direction_output(ltc2952_data->gpio[POWERPATH_IO_KILL], 0);
+ ret = gpiod_direction_output(ltc2952_data->gpio_kill, 0);
if (ret) {
dev_err(&pdev->dev, "unable to use kill-gpio as output\n");
goto err_io;
}

- virq = gpiod_to_irq(ltc2952_data->gpio[POWERPATH_IO_TRIGGER]);
+ virq = gpiod_to_irq(ltc2952_data->gpio_trigger);
if (virq < 0) {
dev_err(&pdev->dev, "cannot map GPIO as interrupt");
goto err_io;
@@ -272,9 +261,11 @@ static int ltc2952_poweroff_init(struct platform_device *pdev)
return 0;

err_io:
- for (i = 0; i < ARRAY_SIZE(ltc2952_data->gpio); i++)
- if (ltc2952_data->gpio[i])
- gpiod_put(ltc2952_data->gpio[i]);
+ gpiod_put(ltc2952_data->gpio_trigger);
+err_trigger:
+ gpiod_put(ltc2952_data->gpio_kill);
+err_kill:
+ gpiod_put(ltc2952_data->gpio_watchdog);

return ret;
}
@@ -308,14 +299,13 @@ static int ltc2952_poweroff_probe(struct platform_device *pdev)

static int ltc2952_poweroff_remove(struct platform_device *pdev)
{
- unsigned int i;
-
pm_power_off = NULL;

- if (ltc2952_data)
- for (i = 0; i < ARRAY_SIZE(ltc2952_data->gpio); i++)
- gpiod_put(ltc2952_data->gpio[i]);
-
+ if (ltc2952_data) {
+ gpiod_put(ltc2952_data->gpio_trigger);
+ gpiod_put(ltc2952_data->gpio_watchdog);
+ gpiod_put(ltc2952_data->gpio_kill);
+ }
return 0;
}
--
2.1.0
Frans Klaver
2014-10-22 14:30:59 UTC
Permalink
Make use of the fact that we allocated resources can be automatically
deallocated. This reduces cleanup code and chance of errors. It also
removes the need for the virq member of the ltc2952_poweroff_data
struct.

Signed-off-by: Frans Klaver <***@xsens.com>
---
drivers/power/reset/ltc2952-poweroff.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/power/reset/ltc2952-poweroff.c b/drivers/power/reset/ltc2952-poweroff.c
index 951a90d..2c11316 100644
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -72,8 +72,6 @@ struct ltc2952_poweroff_data {

struct device *dev;

- unsigned int virq;
-
/**
* 0: trigger
* 1: watchdog
@@ -260,13 +258,11 @@ static int ltc2952_poweroff_init(struct platform_device *pdev)
goto err_io;
}

- ltc2952_data->virq = virq;
- ret = request_irq(virq,
- ltc2952_poweroff_handler,
- (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING),
- "ltc2952-poweroff",
- ltc2952_data
- );
+ ret = devm_request_irq(&pdev->dev, virq,
+ ltc2952_poweroff_handler,
+ (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING),
+ "ltc2952-poweroff",
+ ltc2952_data);

if (ret) {
dev_err(&pdev->dev, "cannot configure an interrupt handler\n");
@@ -316,12 +312,9 @@ static int ltc2952_poweroff_remove(struct platform_device *pdev)

pm_power_off = NULL;

- if (ltc2952_data) {
- free_irq(ltc2952_data->virq, ltc2952_data);
-
+ if (ltc2952_data)
for (i = 0; i < ARRAY_SIZE(ltc2952_data->gpio); i++)
gpiod_put(ltc2952_data->gpio[i]);
- }

return 0;
}
--
2.1.0
Loading...