From: Bruno Thomsen <bruno.thomsen@gmail.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>,
Qiang Zhao <qiang.zhao@nxp.com>,
linux-rtc@vger.kernel.org,
Alessandro Zummo <a.zummo@towertech.it>,
linux-watchdog@vger.kernel.org,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>,
open list <linux-kernel@vger.kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Sascha Hauer <kernel@pengutronix.de>,
Wim Van Sebroeck <wim@linux-watchdog.org>,
Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function
Date: Sun, 27 Sep 2020 09:50:48 +0200 [thread overview]
Message-ID: <CAH+2xPBu3k53XdLzzak7HP_4W16DkfE9NHXCsjaULRbNctzOuQ@mail.gmail.com> (raw)
In-Reply-To: <20200924105256.18162-2-u.kleine-koenig@pengutronix.de>
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
>
next prev parent reply other threads:[~2020-09-27 7:51 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-21 5:48 [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Qiang Zhao
2020-09-21 5:48 ` [Patch v2 2/3] rtc: pcf2127: add "no-watchdog" property Qiang Zhao
2020-09-21 5:48 ` [Patch v2 3/3] arm64: dts: lx2160a: Add "no-watchdog" property to pcf2127 Qiang Zhao
2020-09-23 9:44 ` [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Alexandre Belloni
2020-09-24 3:20 ` Qiang Zhao
2020-09-24 7:04 ` Uwe Kleine-König
2020-09-24 7:23 ` Qiang Zhao
2020-09-24 7:47 ` Alexandre Belloni
2020-09-24 10:52 ` [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
2020-09-24 10:52 ` [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function Uwe Kleine-König
2020-09-27 7:50 ` Bruno Thomsen [this message]
2020-09-24 10:52 ` [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
2020-09-27 8:09 ` Bruno Thomsen
2020-09-27 15:54 ` Guenter Roeck
2020-09-28 8:43 ` Uwe Kleine-König
2020-09-28 16:26 ` Guenter Roeck
2020-10-26 7:23 ` [PATCH 0/2] " Qiang Zhao
2020-10-26 20:48 ` Alexandre Belloni
2020-10-26 21:29 ` Guenter Roeck
2020-11-30 9:51 ` Rasmus Villemoes
2020-12-04 9:27 ` Alexandre Belloni
2020-09-24 16:59 ` [Patch v2 1/3] dt-bindings: rtc: pcf2127: Add bindings for nxp,pcf2127 Bruno Thomsen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAH+2xPBu3k53XdLzzak7HP_4W16DkfE9NHXCsjaULRbNctzOuQ@mail.gmail.com \
--to=bruno.thomsen@gmail.com \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@bootlin.com \
--cc=devicetree@vger.kernel.org \
--cc=kernel@pengutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=qiang.zhao@nxp.com \
--cc=robh+dt@kernel.org \
--cc=u.kleine-koenig@pengutronix.de \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).