Thread (39 messages) 39 messages, 7 authors, 2020-12-19

Re: [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function

From: Bruno Thomsen <hidden>
Date: 2020-09-27 07:51:14
Also in: linux-devicetree, linux-watchdog, lkml

Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
[off-list ref]:
quoted hunk ↗ jump to hunk
The obvious advantages are:

 - The linker can drop the watchdog functions if CONFIG_WATCHDOG is off.
 - All watchdog stuff grouped together with only a single function call
   left in generic code.
 - Watchdog register is only read when it is actually used.
 - Less #ifdefery

Signed-off-by: Uwe Kleine-König <redacted>
---
 drivers/rtc/rtc-pcf2127.c | 56 ++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 25 deletions(-)
diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index ed6316992cbb..5b1f1949b5e5 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -335,6 +335,36 @@ static const struct watchdog_ops pcf2127_watchdog_ops = {
        .set_timeout = pcf2127_wdt_set_timeout,
 };

+static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
+{
+       u32 wdd_timeout;
+       int ret;
+
+       if (!IS_ENABLED(CONFIG_WATCHDOG))
+               return 0;
+
+       pcf2127->wdd.parent = dev;
+       pcf2127->wdd.info = &pcf2127_wdt_info;
+       pcf2127->wdd.ops = &pcf2127_watchdog_ops;
+       pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
+       pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
+       pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
+       pcf2127->wdd.min_hw_heartbeat_ms = 500;
+       pcf2127->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
+
+       watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
+
+       /* Test if watchdog timer is started by bootloader */
+       ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
+       if (ret)
+               return ret;
+
+       if (wdd_timeout)
+               set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
+
+       return devm_watchdog_register_device(dev, &pcf2127->wdd);
+}
+
 /* Alarm */
 static int pcf2127_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
@@ -536,7 +566,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
                         int alarm_irq, const char *name, bool has_nvmem)
 {
        struct pcf2127 *pcf2127;
-       u32 wdd_timeout;
        int ret = 0;

        dev_dbg(dev, "%s\n", __func__);
@@ -575,17 +604,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
                pcf2127->rtc->ops = &pcf2127_rtc_alrm_ops;
        }

-       pcf2127->wdd.parent = dev;
-       pcf2127->wdd.info = &pcf2127_wdt_info;
-       pcf2127->wdd.ops = &pcf2127_watchdog_ops;
-       pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
-       pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
-       pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
-       pcf2127->wdd.min_hw_heartbeat_ms = 500;
-       pcf2127->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
-
-       watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
-
        if (has_nvmem) {
                struct nvmem_config nvmem_cfg = {
                        .priv = pcf2127,
@@ -615,19 +633,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
                return ret;
        }

-       /* Test if watchdog timer is started by bootloader */
-       ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
-       if (ret)
-               return ret;
-
-       if (wdd_timeout)
-               set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
-
-#ifdef CONFIG_WATCHDOG
-       ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
-       if (ret)
-               return ret;
-#endif /* CONFIG_WATCHDOG */
+       pcf2127_watchdog_init(dev, pcf2127);
The code refactoring seems like a good idea Uwe, but the new
pcf2127_watchdog_init() is not a void function. If the return
value is not handled, it will change driver behavior. Correct
handling should look like this:

ret = pcf2127_watchdog_init(dev, pcf2127);
if (ret)
        return ret;

/Bruno
        /*
         * Disable battery low/switch-over timestamp and interrupts.
--
2.28.0
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help