Re: [PATCH v2] rtc: ds1307: add support for watchdog timer on ds1388

[Guenter Roeck <linux@roeck-us.net>]

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;
>  
>