* [PATCH v2] rtc: ds1307: add support for watchdog timer on ds1388
@ 2020-03-27 4:18 Chris Packham
2020-03-27 21:12 ` Guenter Roeck
0 siblings, 1 reply; 2+ messages in thread
From: Chris Packham @ 2020-03-27 4:18 UTC (permalink / raw)
To: a.zummo, alexandre.belloni, wim, linux
Cc: linux-rtc, linux-watchdog, linux-kernel, Chris Packham
The DS1388 variant has watchdog timer capabilities. When using a DS1388
and having enabled CONFIG_WATCHDOG_CORE register a watchdog device for
the DS1388.
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
This is going to the linux-watchdog list as well this time so it's probably the
first time the watchdog maintainers have seen it.
Changes in v2:
- Address review comments from Alexandre, the only functional change is setting
the hundredths of seconds to 0 instead of 99.
drivers/rtc/rtc-ds1307.c | 97 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 97 insertions(+)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 31a38d468378..1452982c3a6a 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -22,6 +22,7 @@
#include <linux/hwmon-sysfs.h>
#include <linux/clk-provider.h>
#include <linux/regmap.h>
+#include <linux/watchdog.h>
/*
* We can't determine type by probing, but if we expect pre-Linux code
@@ -144,8 +145,15 @@ enum ds_type {
# define M41TXX_BIT_CALIB_SIGN BIT(5)
# define M41TXX_M_CALIBRATION GENMASK(4, 0)
+#define DS1388_REG_WDOG_HUN_SECS 0x08
+#define DS1388_REG_WDOG_SECS 0x09
#define DS1388_REG_FLAG 0x0b
+# define DS1388_BIT_WF BIT(6)
# define DS1388_BIT_OSF BIT(7)
+#define DS1388_REG_CONTROL 0x0c
+# define DS1388_BIT_RST BIT(0)
+# define DS1388_BIT_WDE BIT(1)
+
/* negative offset step is -2.034ppm */
#define M41TXX_NEG_OFFSET_STEP_PPB 2034
/* positive offset step is +4.068ppm */
@@ -166,6 +174,9 @@ struct ds1307 {
#ifdef CONFIG_COMMON_CLK
struct clk_hw clks[2];
#endif
+#ifdef CONFIG_WATCHDOG_CORE
+ struct watchdog_device wdt;
+#endif
};
struct chip_desc {
@@ -854,6 +865,58 @@ static int m41txx_rtc_set_offset(struct device *dev, long offset)
ctrl_reg);
}
+#ifdef CONFIG_WATCHDOG_CORE
+static int ds1388_wdt_start(struct watchdog_device *wdt_dev)
+{
+ struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev);
+ u8 regs[2];
+ int ret;
+
+ ret = regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG,
+ DS1388_BIT_WF, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(ds1307->regmap, DS1388_REG_CONTROL,
+ DS1388_BIT_WDE | DS1388_BIT_RST, 0);
+ if (ret)
+ return ret;
+
+ /*
+ * watchdog timeouts are measured in seconds. So ignore hundreths of
+ * seconds field.
+ */
+ regs[0] = 0;
+ regs[1] = bin2bcd(wdt_dev->timeout);
+
+ ret = regmap_bulk_write(ds1307->regmap, DS1388_REG_WDOG_HUN_SECS, regs,
+ sizeof(regs));
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(ds1307->regmap, DS1388_REG_CONTROL,
+ DS1388_BIT_WDE | DS1388_BIT_RST,
+ DS1388_BIT_WDE | DS1388_BIT_RST);
+}
+
+static int ds1388_wdt_stop(struct watchdog_device *wdt_dev)
+{
+ struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev);
+
+ return regmap_update_bits(ds1307->regmap, DS1388_REG_CONTROL,
+ DS1388_BIT_WDE | DS1388_BIT_RST, 0);
+}
+
+static int ds1388_wdt_ping(struct watchdog_device *wdt_dev)
+{
+ struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev);
+ u8 regs[2];
+
+ return regmap_bulk_read(ds1307->regmap, DS1388_REG_WDOG_HUN_SECS, regs,
+ sizeof(regs));
+}
+#endif
+
static const struct rtc_class_ops rx8130_rtc_ops = {
.read_time = ds1307_get_time,
.set_time = ds1307_set_time,
@@ -1576,6 +1639,39 @@ static void ds1307_clks_register(struct ds1307 *ds1307)
#endif /* CONFIG_COMMON_CLK */
+#ifdef CONFIG_WATCHDOG_CORE
+static const struct watchdog_info ds1388_wdt_info = {
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+ .identity = "DS1388 watchdog",
+};
+
+static const struct watchdog_ops ds1388_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = ds1388_wdt_start,
+ .stop = ds1388_wdt_stop,
+ .ping = ds1388_wdt_ping,
+};
+
+static void ds1307_wdt_register(struct ds1307 *ds1307)
+{
+ if (ds1307->type != ds_1388)
+ return;
+
+ ds1307->wdt.info = &ds1388_wdt_info;
+ ds1307->wdt.ops = &ds1388_wdt_ops;
+ ds1307->wdt.max_timeout = 99;
+ ds1307->wdt.min_timeout = 1;
+
+ watchdog_init_timeout(&ds1307->wdt, 99, ds1307->dev);
+ watchdog_set_drvdata(&ds1307->wdt, ds1307);
+ watchdog_register_device(&ds1307->wdt);
+}
+#else
+static void ds1307_wdt_register(struct ds1307 *ds1307)
+{
+}
+#endif /* CONFIG_WATCHDOG_CORE */
+
static const struct regmap_config regmap_config = {
.reg_bits = 8,
.val_bits = 8,
@@ -1865,6 +1961,7 @@ static int ds1307_probe(struct i2c_client *client,
ds1307_hwmon_register(ds1307);
ds1307_clks_register(ds1307);
+ ds1307_wdt_register(ds1307);
return 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH v2] rtc: ds1307: add support for watchdog timer on ds1388
2020-03-27 4:18 [PATCH v2] rtc: ds1307: add support for watchdog timer on ds1388 Chris Packham
@ 2020-03-27 21:12 ` Guenter Roeck
0 siblings, 0 replies; 2+ messages in thread
From: Guenter Roeck @ 2020-03-27 21:12 UTC (permalink / raw)
To: Chris Packham, a.zummo, alexandre.belloni, wim
Cc: linux-rtc, linux-watchdog, linux-kernel
On 3/26/20 9:18 PM, Chris Packham wrote:
> The DS1388 variant has watchdog timer capabilities. When using a DS1388
> and having enabled CONFIG_WATCHDOG_CORE register a watchdog device for
> the DS1388.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> This is going to the linux-watchdog list as well this time so it's probably the
> first time the watchdog maintainers have seen it.
>
> Changes in v2:
> - Address review comments from Alexandre, the only functional change is setting
> the hundredths of seconds to 0 instead of 99.
>
> drivers/rtc/rtc-ds1307.c | 97 ++++++++++++++++++++++++++++++++++++++++
"select WATCHDOG_CORE if WATCHDOG"
should be added to Kconfig. While it makes sense for watchdog functionality
to depend on WATCHDOG, it should not depend on the existence of another
watchdog driver in the system.
> 1 file changed, 97 insertions(+)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 31a38d468378..1452982c3a6a 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -22,6 +22,7 @@
> #include <linux/hwmon-sysfs.h>
> #include <linux/clk-provider.h>
> #include <linux/regmap.h>
> +#include <linux/watchdog.h>
>
> /*
> * We can't determine type by probing, but if we expect pre-Linux code
> @@ -144,8 +145,15 @@ enum ds_type {
> # define M41TXX_BIT_CALIB_SIGN BIT(5)
> # define M41TXX_M_CALIBRATION GENMASK(4, 0)
>
> +#define DS1388_REG_WDOG_HUN_SECS 0x08
> +#define DS1388_REG_WDOG_SECS 0x09
> #define DS1388_REG_FLAG 0x0b
> +# define DS1388_BIT_WF BIT(6)
> # define DS1388_BIT_OSF BIT(7)
> +#define DS1388_REG_CONTROL 0x0c
> +# define DS1388_BIT_RST BIT(0)
> +# define DS1388_BIT_WDE BIT(1)
> +
> /* negative offset step is -2.034ppm */
> #define M41TXX_NEG_OFFSET_STEP_PPB 2034
> /* positive offset step is +4.068ppm */
> @@ -166,6 +174,9 @@ struct ds1307 {
> #ifdef CONFIG_COMMON_CLK
> struct clk_hw clks[2];
> #endif
> +#ifdef CONFIG_WATCHDOG_CORE
> + struct watchdog_device wdt;
> +#endif
I don't immediately see why this would be necessary. I think it would
be better to allocate struct watchdog_device in ds1307_wdt_register().
> };
>
> struct chip_desc {
> @@ -854,6 +865,58 @@ static int m41txx_rtc_set_offset(struct device *dev, long offset)
> ctrl_reg);
> }
>
> +#ifdef CONFIG_WATCHDOG_CORE
> +static int ds1388_wdt_start(struct watchdog_device *wdt_dev)
> +{
> + struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev);
> + u8 regs[2];
> + int ret;
> +
> + ret = regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG,
> + DS1388_BIT_WF, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(ds1307->regmap, DS1388_REG_CONTROL,
> + DS1388_BIT_WDE | DS1388_BIT_RST, 0);
> + if (ret)
> + return ret;
> +
> + /*
> + * watchdog timeouts are measured in seconds. So ignore hundreths of
hundredths
> + * seconds field.
> + */
> + regs[0] = 0;
> + regs[1] = bin2bcd(wdt_dev->timeout);
> +
> + ret = regmap_bulk_write(ds1307->regmap, DS1388_REG_WDOG_HUN_SECS, regs,
> + sizeof(regs));
> + if (ret)
> + return ret;
> +
> + return regmap_update_bits(ds1307->regmap, DS1388_REG_CONTROL,
> + DS1388_BIT_WDE | DS1388_BIT_RST,
> + DS1388_BIT_WDE | DS1388_BIT_RST);
> +}
> +
> +static int ds1388_wdt_stop(struct watchdog_device *wdt_dev)
> +{
> + struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev);
> +
> + return regmap_update_bits(ds1307->regmap, DS1388_REG_CONTROL,
> + DS1388_BIT_WDE | DS1388_BIT_RST, 0);
> +}
> +
> +static int ds1388_wdt_ping(struct watchdog_device *wdt_dev)
> +{
> + struct ds1307 *ds1307 = watchdog_get_drvdata(wdt_dev);
> + u8 regs[2];
> +
> + return regmap_bulk_read(ds1307->regmap, DS1388_REG_WDOG_HUN_SECS, regs,
> + sizeof(regs));
> +}
> +#endif
> +
> static const struct rtc_class_ops rx8130_rtc_ops = {
> .read_time = ds1307_get_time,
> .set_time = ds1307_set_time,
> @@ -1576,6 +1639,39 @@ static void ds1307_clks_register(struct ds1307 *ds1307)
>
> #endif /* CONFIG_COMMON_CLK */
>
> +#ifdef CONFIG_WATCHDOG_CORE
> +static const struct watchdog_info ds1388_wdt_info = {
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> + .identity = "DS1388 watchdog",
> +};
> +
> +static const struct watchdog_ops ds1388_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = ds1388_wdt_start,
> + .stop = ds1388_wdt_stop,
> + .ping = ds1388_wdt_ping,
Maybe I am missing something, but I don't see how the timeout is updated
if it is changed while the watchdog is already running.
> +};
> +
> +static void ds1307_wdt_register(struct ds1307 *ds1307)
> +{
> + if (ds1307->type != ds_1388)
> + return;
> +
> + ds1307->wdt.info = &ds1388_wdt_info;
> + ds1307->wdt.ops = &ds1388_wdt_ops;
> + ds1307->wdt.max_timeout = 99;
> + ds1307->wdt.min_timeout = 1;
> +
> + watchdog_init_timeout(&ds1307->wdt, 99, ds1307->dev);
That is quite pointless; just set wdt.timeout to 99 (assuming
that is what you want as default).
watchdog_init_timeout() only makes sense if it is used to set the
timeout from a module parameter or from a devicetree property.
> + watchdog_set_drvdata(&ds1307->wdt, ds1307);
> + watchdog_register_device(&ds1307->wdt);
Please call devm_watchdog_register_device().
> +}
> +#else
> +static void ds1307_wdt_register(struct ds1307 *ds1307)
> +{
> +}
> +#endif /* CONFIG_WATCHDOG_CORE */
> +
> static const struct regmap_config regmap_config = {
> .reg_bits = 8,
> .val_bits = 8,
> @@ -1865,6 +1961,7 @@ static int ds1307_probe(struct i2c_client *client,
>
> ds1307_hwmon_register(ds1307);
> ds1307_clks_register(ds1307);
> + ds1307_wdt_register(ds1307);
>
> return 0;
>
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-03-27 21:12 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-27 4:18 [PATCH v2] rtc: ds1307: add support for watchdog timer on ds1388 Chris Packham
2020-03-27 21:12 ` Guenter Roeck
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).