Message-ID: <06fbcdd3-4368-44e0-9a31-6cf83fa625d5@roeck-us.net>
Date: Wed, 19 Nov 2025 07:37:23 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Lakshay Piplani <lakshay.piplani@....com>, alexandre.belloni@...tlin.com,
 linux-rtc@...r.kernel.org, linux-kernel@...r.kernel.org, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, devicetree@...r.kernel.org,
 wim@...ux-watchdog.org, linux-watchdog@...r.kernel.org
Cc: vikash.bansal@....com, priyanka.jain@....com,
 shashank.rebbapragada@....com
Subject: Re: [PATCH v3 5/5] rtc: pcf85363: add watchdog support with
 configurable step size

On 11/19/25 00:33, Lakshay Piplani wrote:
> Add watchdog timer support to PCF85263/PCF85363 using the linux watchdog
> subsystem. The driver programs the hardware watchdog timeout based on
> the requested period.
> 
> Also use rtc_add_group() instead of sysfs_create_group() to register
> timestamp attributes under the RTC class device (/sys/class/rtc/rtcX).
> 
> Signed-off-by: Lakshay Piplani <lakshay.piplani@....com>
> ---
> V2 -> V3:
> - Split into separate patches as suggested:
>    - Battery switch-over detection.
>    - Timestamp recording for TS pin and battery switch-over events.
>    - Offset calibration.
>    - Watchdog timer (to be reviewed by watchdog maintainers).
> - Dropped Alarm2 support
> - Switched to rtc_add_group() for sysfs attributes
> - Removed failure paths after RTC device registration as per subsystem guidelines.
> V1 -> V2:
> - Watchdog related changes due to removal of vendor specific properties
>    from device tree
>    * remove vendor DT knobs (enable/timeout/stepsize/repeat)
>    * use watchdog_init_timeout (with 10s default)
>    * derive clock_sel from final timeout
>    * default, repeat=true (repeat mode)
> - Fixed uninitalised warning on 'ret' (reported by kernel test robot)
> - Use dev_dbg instead of dev_info for debug related print messages
> - Minor cleanup and comments
> 
>   drivers/rtc/rtc-pcf85363.c | 168 +++++++++++++++++++++++++++++++++++--
>   1 file changed, 160 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index 3d733375187b..34d4c2e16774 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
> @@ -5,6 +5,10 @@
>    * Driver for NXP PCF85363 real-time clock.
>    *
>    * Copyright (C) 2017 Eric Nelson
> + *
> + * Copyright 2025 NXP
> + * Added support for timestamps, battery switch-over,
> + * watchdog, offset calibration.
>    */
>   #include <linux/module.h>
>   #include <linux/i2c.h>
> @@ -17,6 +21,8 @@
>   #include <linux/device.h>
>   #include <linux/of.h>
>   #include <linux/regmap.h>
> +#include <linux/rtc.h>
> +#include <linux/watchdog.h>
>   
>   /*
>    * Date/Time registers
> @@ -127,6 +133,18 @@
>   #define OFFSET_MAXIMUM  127
>   #define OFFSET_MASK     0xFF
>   
> +#define WD_MODE_REPEAT  BIT(7)
> +#define WD_TIMEOUT_MASK GENMASK(6, 2)
> +#define WD_TIMEOUT_SHIFT        2
> +#define WD_CLKSEL_MASK  GENMASK(1, 0)
> +#define WD_CLKSEL_0_25HZ        0x00
> +#define WD_CLKSEL_1HZ   0x01
> +#define WD_CLKSEL_4HZ   0x02
> +#define WD_CLKSEL_16HZ  0x03
> +
> +#define WD_TIMEOUT_MIN  1
> +#define WD_TIMEOUT_MAX  0x1F
> +
>   struct pcf85363 {
>   	struct rtc_device	*rtc;
>   	struct regmap		*regmap;
> @@ -138,6 +156,15 @@ struct pcf85x63_config {
>   	unsigned int num_nvram;
>   };
>   
> +struct pcf85363_watchdog {
> +	struct watchdog_device wdd;
> +	struct regmap *regmap;
> +	struct device *dev;
> +	u8 timeout_val;
> +	u8 clock_sel;
> +	bool repeat;
> +};
> +
>   static int pcf85363_load_capacitance(struct pcf85363 *pcf85363, struct device_node *node)
>   {
>   	u32 load = 7000;
> @@ -323,12 +350,13 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
>   		return IRQ_NONE;
>   
>   	if (flags) {
> -		dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s%s%s%s\n",
> +		dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s%s%s%s%s\n",
>   			flags, (flags & FLAGS_A1F) ? " [A1F]" : "",
>   			(flags & FLAGS_TSR1F) ? " [TSR1F]" : "",
>   			(flags & FLAGS_TSR2F) ? " [TSR2F]" : "",
>   			(flags & FLAGS_TSR3F) ? " [TSR3F]" : "",
> -			(flags & FLAGS_BSF) ? " [BSF]" : "");
> +			(flags & FLAGS_BSF) ? " [BSF]" : "",
> +			(flags & FLAGS_WDF) ? " [WDF]" : "");
>   	}
>   
>   	if (flags & FLAGS_A1F) {
> @@ -360,6 +388,11 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
>   		handled = true;
>   	}
>   
> +	if (flags & FLAGS_WDF) {
> +		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_WDF, 0);
> +		handled = true;
> +	}
> +
>   	return handled ? IRQ_HANDLED : IRQ_NONE;
>   }
>   
> @@ -503,6 +536,123 @@ static const struct pcf85x63_config pcf_85363_config = {
>   	.num_nvram = 2
>   };
>   
> +/*
> + * This function sets the watchdog control register based on the timeout,
> + * clock selection and repeat mode settings. It prepares the value to
> + * write into the watchdog control register (CTRL_WDOG).
> + */
> +static int pcf85363_wdt_reload(struct pcf85363_watchdog *wd)
> +{
> +	u8 val;
> +
> +	val = (wd->repeat ? WD_MODE_REPEAT : 0) |
> +	       ((wd->timeout_val & WD_TIMEOUT_MAX) << WD_TIMEOUT_SHIFT) |
> +	       (wd->clock_sel & WD_CLKSEL_MASK);
> +
> +	return regmap_write(wd->regmap, CTRL_WDOG, val);
> +}
> +
> +static int pcf85363_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
> +
> +	return pcf85363_wdt_reload(wd);
> +}
> +
> +static int pcf85363_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
> +
> +	return regmap_write(wd->regmap, CTRL_WDOG, 0);
> +}
> +
> +static int pcf85363_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
> +
> +	regmap_update_bits(wd->regmap, CTRL_FLAGS, FLAGS_WDF, 0);
> +
> +	return pcf85363_wdt_reload(wd);
> +}
> +
> +static int pcf85363_wdt_set_timeout(struct watchdog_device *wdd,
> +				    unsigned int timeout)
> +{
> +	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
> +
> +	wd->timeout_val = clamp(timeout, WD_TIMEOUT_MIN, WD_TIMEOUT_MAX);
> +	wdd->timeout = wd->timeout_val;
> +
> +	return pcf85363_wdt_reload(wd);
> +}
> +
> +static const struct watchdog_info pcf85363_wdt_info = {
> +	.identity = "PCF85363 Watchdog",
> +	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> +};
> +
> +static const struct watchdog_ops pcf85363_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = pcf85363_wdt_start,
> +	.stop = pcf85363_wdt_stop,
> +	.ping = pcf85363_wdt_ping,
> +	.set_timeout = pcf85363_wdt_set_timeout,
> +};
> +
> +static int pcf85363_watchdog_init(struct device *dev, struct regmap *regmap)
> +{
> +	struct pcf85363_watchdog *wd;
> +	unsigned int timeout_sec;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_WATCHDOG))
> +		return 0;
> +
> +	wd = devm_kzalloc(dev, sizeof(*wd), GFP_KERNEL);
> +	if (!wd)
> +		return -ENOMEM;
> +
> +	wd->regmap = regmap;
> +	wd->dev = dev;
> +
> +	wd->wdd.info = &pcf85363_wdt_info;
> +	wd->wdd.ops = &pcf85363_wdt_ops;
> +	wd->wdd.min_timeout = WD_TIMEOUT_MIN;
> +	wd->wdd.max_timeout = WD_TIMEOUT_MAX;
> +	wd->wdd.parent = dev;
> +	wd->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> +
> +	ret = watchdog_init_timeout(&wd->wdd, 10, dev);

Calling watchdog_init_timeout() with a value other than 0 means that
a parameter from devicetree won't be accepted. Calling it with a fixed
value is usually pointless unless the value is out of the valid range,
which by itself would be pointless.

watchdog_init_timeout() is normally called to pass and validate a module
parameter or to pick a timeout from devicetree. Calling it with a constant
value other than 0 is unnecessary.

> +	if (ret)
> +		wd->wdd.timeout = clamp(10U, WD_TIMEOUT_MIN, WD_TIMEOUT_MAX);

So if 10 seconds is invalid, 10 is clamped to [1, 31] and applied directly.
That is an odd and complicated way of setting the timeout to 10 seconds.

If you don't want a timeout value from devicetree to be accepted, just make this

	wd->wdd.timeout = 10;

and do not call watchdog_init_timeout() in the first place.

> +
> +	timeout_sec = wd->wdd.timeout;
> +
> +	if (timeout_sec <= 2)
> +		wd->clock_sel = WD_CLKSEL_16HZ;
> +	else if (timeout_sec <= 8)
> +		wd->clock_sel = WD_CLKSEL_4HZ;
> +	else if (timeout_sec <= 16)
> +		wd->clock_sel = WD_CLKSEL_1HZ;
> +	else
> +		wd->clock_sel = WD_CLKSEL_0_25HZ;
> +

This seems an odd location for this code. What if the timeout changes
later on to one of the other values ?

Also, the timeout is set to a fixed value of 10. That means the above
can be simplified to
	wd->clock_sel = WD_CLKSEL_1HZ;
... and that in turn means that the variable is pointless, and that
WD_CLKSEL_1HZ could be used as constant instead.

Why all that complexity ? Am I missing something ? I am quite concerned that
I may be missing trees in the forest, meaning that the real problems are hiding
behind the noise.

Guenter

> +	wd->repeat = true;

What is the purpose of this variable ? It is always set to true.
You might as well drop it.

> +
> +	ret = regmap_update_bits(regmap, CTRL_FLAGS, FLAGS_WDF, 0);
> +	if (ret) {
> +		dev_err(dev, "failed to clear WDF:%d\n", ret);
> +		return ret;
> +	}
> +
> +	watchdog_set_drvdata(&wd->wdd, wd);
> +
> +	dev_dbg(dev, "pcf85363 watchdog registered (timeout=%us, clk_sel=%u)\n",
> +		timeout_sec, wd->clock_sel);
> +
> +	return devm_watchdog_register_device(dev, &wd->wdd);
> +}
> +
>   /*
>    * Reads 6 bytes of timestamp data starting at the given base register,
>    * converts them from BCD to binary, and formats the result into a
> @@ -684,20 +834,22 @@ static int pcf85363_probe(struct i2c_client *client)
>   			   PIN_IO_TSPM | PIN_IO_TSIM,
>   			   PIN_IO_TSPM | PIN_IO_TSIM);
>   
> +	ret = pcf85363_watchdog_init(dev, pcf85363->regmap);
> +	if (ret)
> +		dev_err_probe(dev, ret, "Watchdog init failed\n");
> +
>   	if (irq_a > 0 || wakeup_source)
>   		device_init_wakeup(dev, true);
>   
>   	dev_set_drvdata(&pcf85363->rtc->dev, pcf85363);
>   
> -	ret = devm_rtc_register_device(pcf85363->rtc);
> -
> +	ret = rtc_add_group(pcf85363->rtc, &pcf85363_attr_group);
>   	if (ret)
> -		return dev_err_probe(dev, ret, "RTC registration failed\n");
> -
> -	ret = sysfs_create_group(&pcf85363->rtc->dev.kobj, &pcf85363_attr_group);
> +		return ret;
>   
> +	ret = devm_rtc_register_device(pcf85363->rtc);
>   	if (ret)
> -		return dev_err_probe(dev, ret, "Timestamp sysfs creation failed\n");
> +		return dev_err_probe(dev, ret, "RTC registration failed\n");
>   
It is not entirely obvious how those changes are related to adding watchdog support
to this driver.

>   	for (i = 0; i < config->num_nvram; i++) {
>   		nvmem_cfg[i].priv = pcf85363;

Powered by blists - more mailing lists