diff mbox series

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

Message ID 20200924105256.18162-2-u.kleine-koenig@pengutronix.de (mailing list archive)
State Not Applicable
Headers show
Series rtc: pcf2127: only use watchdog when explicitly available | expand

Commit Message

Uwe Kleine-König Sept. 24, 2020, 10:52 a.m. UTC 
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 <u.kleine-koenig@pengutronix.de>
---
 drivers/rtc/rtc-pcf2127.c | 56 ++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 25 deletions(-)

Comments

Bruno Thomsen Sept. 27, 2020, 7:50 a.m. UTC | #1 
Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
<u.kleine-koenig@pengutronix.de>:
>
> 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 <u.kleine-koenig@pengutronix.de>
> ---
>  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
>
diff mbox series

Patch

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);
 
 	/*
 	 * Disable battery low/switch-over timestamp and interrupts.