[RFC PATCH v2 10/10] watchdog: bd70528: Initial support for ROHM BD70528 watchdog block

[RFC PATCH v2 10/10] watchdog: bd70528: Initial support for ROHM BD70528 watchdog block

[Matti Vaittinen]

Initial support for watchdog block included in ROHM BD70528
power management IC.

Configurations for low power states are still to be checked.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/watchdog/Kconfig       |  12 +++
 drivers/watchdog/Makefile      |   1 +
 drivers/watchdog/bd70528_wdt.c | 183 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 196 insertions(+)
 create mode 100644 drivers/watchdog/bd70528_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 57f017d74a97..f30e3a3e886e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -90,6 +90,18 @@ config SOFT_WATCHDOG_PRETIMEOUT
 	  watchdog. Be aware that governors might affect the watchdog because it
 	  is purely software, e.g. the panic governor will stall it!
 
+config BD70528_WATCHDOG
+	tristate "ROHM BD70528 PMIC Watchdog"
+	depends on MFD_ROHM_BD70528
+	select WATCHDOG_CORE
+	help
+	  Support for the watchdog in the ROHM BD70528 PMIC. Watchdog trigger
+	  cause system reset.
+
+	  Say Y here to include support for the ROHM BD70528 watchdog.
+	  Alternatively say M to compile the driver as a module,
+	  which will be called bd70528_wdt.
+
 config DA9052_WATCHDOG
 	tristate "Dialog DA9052 Watchdog"
 	depends on PMIC_DA9052 || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index a0917ef28e07..1ce87a3b1172 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -204,6 +204,7 @@ obj-$(CONFIG_WATCHDOG_SUN4V)		+= sun4v_wdt.o
 obj-$(CONFIG_XEN_WDT) += xen_wdt.o
 
 # Architecture Independent
+obj-$(CONFIG_BD70528_WATCHDOG) += bd70528_wdt.o
 obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
 obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o
 obj-$(CONFIG_DA9062_WATCHDOG) += da9062_wdt.o
diff --git a/drivers/watchdog/bd70528_wdt.c b/drivers/watchdog/bd70528_wdt.c
new file mode 100644
index 000000000000..347339683cb9
--- /dev/null
+++ b/drivers/watchdog/bd70528_wdt.c
@@ -0,0 +1,183 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// ROHM BD70528MWV watchdog driver
+
+#include <linux/bcd.h>
+#include <linux/kernel.h>
+#include <linux/mfd/rohm-bd70528.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/watchdog.h>
+
+struct wdtbd70528 {
+	struct bd70528 *bd;
+	struct device *dev;
+};
+
+static int bd70528_wdt_set(struct bd70528 *bd70528, int enable)
+{
+	int ret;
+
+	mutex_lock(&bd70528->rtc_timer_lock);
+	ret = bd70528->wdt_set(bd70528, enable, NULL);
+	mutex_unlock(&bd70528->rtc_timer_lock);
+
+	return ret;
+}
+
+static int bd70528_wdt_start(struct watchdog_device *wdt)
+{
+	struct wdtbd70528 *w = watchdog_get_drvdata(wdt);
+
+	dev_dbg(w->dev, "WDT ping...\n");
+	return bd70528_wdt_set(w->bd, 1);
+}
+
+static int bd70528_wdt_stop(struct watchdog_device *wdt)
+{
+	struct wdtbd70528 *w = watchdog_get_drvdata(wdt);
+
+	dev_dbg(w->dev, "WDT stopping...\n");
+	return bd70528_wdt_set(w->bd, 0);
+}
+
+static int bd70528_wdt_set_timeout(struct watchdog_device *wdt,
+				    unsigned int timeout)
+{
+	unsigned int hours;
+	unsigned int minutes;
+	unsigned int seconds;
+	int ret;
+	struct bd70528 *bd70528;
+	struct wdtbd70528 *w = watchdog_get_drvdata(wdt);
+
+	bd70528 = w->bd;
+	seconds = timeout;
+	hours = timeout / (60 * 60);
+	/* Maximum timeout is 1h 59m 59s => hours is 1 or 0 */
+	if (hours)
+		seconds -= (60 * 60);
+	minutes = seconds / 60;
+	seconds = seconds % 60;
+
+	mutex_lock(&bd70528->rtc_timer_lock);
+
+	ret = bd70528->wdt_set(bd70528, 0, NULL);
+	if (ret)
+		goto out_unlock;
+
+	ret = regmap_update_bits(bd70528->chip.regmap, BD70528_REG_WDT_HOUR,
+				 BD70528_MASK_WDT_HOUR, hours);
+	if (ret) {
+		dev_err(w->dev, "Failed to set WDT hours\n");
+		goto out_en_unlock;
+	}
+	ret = regmap_update_bits(bd70528->chip.regmap, BD70528_REG_WDT_MINUTE,
+				 BD70528_MASK_WDT_MINUTE, bin2bcd(minutes));
+	if (ret) {
+		dev_err(w->dev, "Failed to set WDT minutes\n");
+		goto out_en_unlock;
+	}
+	ret = regmap_update_bits(bd70528->chip.regmap, BD70528_REG_WDT_SEC,
+				 BD70528_MASK_WDT_SEC, bin2bcd(seconds));
+	if (ret)
+		dev_err(w->dev, "Failed to set WDT seconds\n");
+	else
+		dev_dbg(w->dev, "WDT tmo set to %u\n", timeout);
+
+out_en_unlock:
+	ret = bd70528->wdt_set(bd70528, 1, NULL);
+out_unlock:
+	mutex_unlock(&bd70528->rtc_timer_lock);
+
+	return ret;
+}
+
+static const struct watchdog_info bd70528_wdt_info = {
+	.identity = "bd70528-wdt",
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+};
+
+static const struct watchdog_ops bd70528_wdt_ops = {
+	.start		= bd70528_wdt_start,
+	.stop		= bd70528_wdt_stop,
+	.set_timeout	= bd70528_wdt_set_timeout,
+};
+
+/* Max time we can set is 1 hour, 59 minutes and 59 seconds */
+#define WDT_MAX_MS ((2 * 60 * 60 - 1) * 1000)
+/* Minimum time is 1 second */
+#define WDT_MIN_MS 1000
+#define DEFAULT_TIMEOUT 60
+
+static int bd70528_wdt_probe(struct platform_device *pdev)
+{
+	struct bd70528 *bd70528;
+	struct wdtbd70528 *w;
+	int ret;
+	unsigned int reg;
+	struct watchdog_device *wdt;
+
+	bd70528 = dev_get_drvdata(pdev->dev.parent);
+	if (!bd70528) {
+		dev_err(&pdev->dev, "No MFD driver data\n");
+		return -EINVAL;
+	}
+	w = devm_kzalloc(&pdev->dev, sizeof(*w), GFP_KERNEL);
+	if (!w)
+		return -ENOMEM;
+	w->bd = bd70528;
+	w->dev = &pdev->dev;
+
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	wdt->info = &bd70528_wdt_info;
+	wdt->ops =  &bd70528_wdt_ops;
+	wdt->min_hw_heartbeat_ms = WDT_MIN_MS;
+	wdt->max_hw_heartbeat_ms = WDT_MAX_MS;
+	wdt->parent = pdev->dev.parent;
+	wdt->timeout = DEFAULT_TIMEOUT;
+	watchdog_set_drvdata(wdt, w);
+	watchdog_init_timeout(wdt, 0, pdev->dev.parent);
+
+	ret = bd70528_wdt_set_timeout(wdt, wdt->timeout);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to set the watchdog timeout\n");
+		return ret;
+	}
+
+	mutex_lock(&bd70528->rtc_timer_lock);
+	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WDT_CTRL, &reg);
+	mutex_unlock(&bd70528->rtc_timer_lock);
+
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to get the watchdog state\n");
+		return ret;
+	}
+	if (reg & BD70528_MASK_WDT_EN) {
+		dev_dbg(&pdev->dev, "watchdog was running during probe\n");
+		set_bit(WDOG_HW_RUNNING, &wdt->status);
+	}
+
+	ret = devm_watchdog_register_device(&pdev->dev, wdt);
+	if (ret < 0)
+		dev_err(&pdev->dev, "watchdog registration failed: %d\n", ret);
+
+	return ret;
+}
+static struct platform_driver bd70528_wdt = {
+	.driver = {
+		.name = "bd70528-wdt"
+	},
+	.probe = bd70528_wdt_probe,
+};
+
+module_platform_driver(bd70528_wdt);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD70528 watchdog driver");
+MODULE_LICENSE("GPL");
-- 
2.14.3


-- 
Matti Vaittinen
ROHM Semiconductors

~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~

Re: [RFC PATCH v2 10/10] watchdog: bd70528: Initial support for ROHM BD70528 watchdog block

[Guenter Roeck]

On 1/25/19 3:06 AM, Matti Vaittinen wrote:
> Initial support for watchdog block included in ROHM BD70528
> power management IC.
> 
> Configurations for low power states are still to be checked.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>   drivers/watchdog/Kconfig       |  12 +++
>   drivers/watchdog/Makefile      |   1 +
>   drivers/watchdog/bd70528_wdt.c | 183 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 196 insertions(+)
>   create mode 100644 drivers/watchdog/bd70528_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 57f017d74a97..f30e3a3e886e 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -90,6 +90,18 @@ config SOFT_WATCHDOG_PRETIMEOUT
>   	  watchdog. Be aware that governors might affect the watchdog because it
>   	  is purely software, e.g. the panic governor will stall it!
>   
> +config BD70528_WATCHDOG
> +	tristate "ROHM BD70528 PMIC Watchdog"
> +	depends on MFD_ROHM_BD70528
> +	select WATCHDOG_CORE
> +	help
> +	  Support for the watchdog in the ROHM BD70528 PMIC. Watchdog trigger
> +	  cause system reset.
> +
> +	  Say Y here to include support for the ROHM BD70528 watchdog.
> +	  Alternatively say M to compile the driver as a module,
> +	  which will be called bd70528_wdt.
> +
>   config DA9052_WATCHDOG
>   	tristate "Dialog DA9052 Watchdog"
>   	depends on PMIC_DA9052 || COMPILE_TEST
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index a0917ef28e07..1ce87a3b1172 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -204,6 +204,7 @@ obj-$(CONFIG_WATCHDOG_SUN4V)		+= sun4v_wdt.o
>   obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>   
>   # Architecture Independent
> +obj-$(CONFIG_BD70528_WATCHDOG) += bd70528_wdt.o
>   obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
>   obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o
>   obj-$(CONFIG_DA9062_WATCHDOG) += da9062_wdt.o
> diff --git a/drivers/watchdog/bd70528_wdt.c b/drivers/watchdog/bd70528_wdt.c
> new file mode 100644
> index 000000000000..347339683cb9
> --- /dev/null
> +++ b/drivers/watchdog/bd70528_wdt.c
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 ROHM Semiconductors
> +// ROHM BD70528MWV watchdog driver
> +
> +#include <linux/bcd.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/rohm-bd70528.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/watchdog.h>
> +
> +struct wdtbd70528 {
> +	struct bd70528 *bd;
> +	struct device *dev;
> +};
> +
> +static int bd70528_wdt_set(struct bd70528 *bd70528, int enable)
> +{
> +	int ret;
> +
> +	mutex_lock(&bd70528->rtc_timer_lock);
> +	ret = bd70528->wdt_set(bd70528, enable, NULL);
> +	mutex_unlock(&bd70528->rtc_timer_lock);
> +
> +	return ret;
> +}
> +
> +static int bd70528_wdt_start(struct watchdog_device *wdt)
> +{
> +	struct wdtbd70528 *w = watchdog_get_drvdata(wdt);
> +
> +	dev_dbg(w->dev, "WDT ping...\n");
> +	return bd70528_wdt_set(w->bd, 1);
> +}
> +
> +static int bd70528_wdt_stop(struct watchdog_device *wdt)
> +{
> +	struct wdtbd70528 *w = watchdog_get_drvdata(wdt);
> +
> +	dev_dbg(w->dev, "WDT stopping...\n");
> +	return bd70528_wdt_set(w->bd, 0);
> +}
> +
> +static int bd70528_wdt_set_timeout(struct watchdog_device *wdt,
> +				    unsigned int timeout)
> +{
> +	unsigned int hours;
> +	unsigned int minutes;
> +	unsigned int seconds;
> +	int ret;
> +	struct bd70528 *bd70528;
> +	struct wdtbd70528 *w = watchdog_get_drvdata(wdt);
> +
> +	bd70528 = w->bd;
> +	seconds = timeout;
> +	hours = timeout / (60 * 60);
> +	/* Maximum timeout is 1h 59m 59s => hours is 1 or 0 */
> +	if (hours)
> +		seconds -= (60 * 60);
> +	minutes = seconds / 60;
> +	seconds = seconds % 60;
> +
> +	mutex_lock(&bd70528->rtc_timer_lock);
> +
> +	ret = bd70528->wdt_set(bd70528, 0, NULL);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = regmap_update_bits(bd70528->chip.regmap, BD70528_REG_WDT_HOUR,
> +				 BD70528_MASK_WDT_HOUR, hours);
> +	if (ret) {
> +		dev_err(w->dev, "Failed to set WDT hours\n");
> +		goto out_en_unlock;
> +	}
> +	ret = regmap_update_bits(bd70528->chip.regmap, BD70528_REG_WDT_MINUTE,
> +				 BD70528_MASK_WDT_MINUTE, bin2bcd(minutes));
> +	if (ret) {
> +		dev_err(w->dev, "Failed to set WDT minutes\n");
> +		goto out_en_unlock;
> +	}
> +	ret = regmap_update_bits(bd70528->chip.regmap, BD70528_REG_WDT_SEC,
> +				 BD70528_MASK_WDT_SEC, bin2bcd(seconds));
> +	if (ret)
> +		dev_err(w->dev, "Failed to set WDT seconds\n");
> +	else
> +		dev_dbg(w->dev, "WDT tmo set to %u\n", timeout);
> +
> +out_en_unlock:
> +	ret = bd70528->wdt_set(bd70528, 1, NULL);
> +out_unlock:
> +	mutex_unlock(&bd70528->rtc_timer_lock);
> +
> +	return ret;
> +}
> +
> +static const struct watchdog_info bd70528_wdt_info = {
> +	.identity = "bd70528-wdt",
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +};
> +
> +static const struct watchdog_ops bd70528_wdt_ops = {
> +	.start		= bd70528_wdt_start,
> +	.stop		= bd70528_wdt_stop,
> +	.set_timeout	= bd70528_wdt_set_timeout,
> +};
> +
> +/* Max time we can set is 1 hour, 59 minutes and 59 seconds */
> +#define WDT_MAX_MS ((2 * 60 * 60 - 1) * 1000)
> +/* Minimum time is 1 second */
> +#define WDT_MIN_MS 1000
> +#define DEFAULT_TIMEOUT 60
> +
> +static int bd70528_wdt_probe(struct platform_device *pdev)
> +{
> +	struct bd70528 *bd70528;
> +	struct wdtbd70528 *w;
> +	int ret;
> +	unsigned int reg;
> +	struct watchdog_device *wdt;
> +
> +	bd70528 = dev_get_drvdata(pdev->dev.parent);
> +	if (!bd70528) {
> +		dev_err(&pdev->dev, "No MFD driver data\n");
> +		return -EINVAL;
> +	}
> +	w = devm_kzalloc(&pdev->dev, sizeof(*w), GFP_KERNEL);
> +	if (!w)
> +		return -ENOMEM;
> +	w->bd = bd70528;

Unless I am missing something, only the mutex and the regmap pointer
are used from struct bd70528. With that in mind, it might be desirable
to copy those pointers to struct wdtbd70528 to avoid the additional
dereferencing at runtime.

> +	w->dev = &pdev->dev;
> +
> +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +

struct watchdog_device can be part of struct wdtbd70528. Two separate allocations
are not necessary.

> +	wdt->info = &bd70528_wdt_info;
> +	wdt->ops =  &bd70528_wdt_ops;
> +	wdt->min_hw_heartbeat_ms = WDT_MIN_MS;
> +	wdt->max_hw_heartbeat_ms = WDT_MAX_MS;
> +	wdt->parent = pdev->dev.parent;
> +	wdt->timeout = DEFAULT_TIMEOUT;
> +	watchdog_set_drvdata(wdt, w);
> +	watchdog_init_timeout(wdt, 0, pdev->dev.parent);
> +
> +	ret = bd70528_wdt_set_timeout(wdt, wdt->timeout);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to set the watchdog timeout\n");
> +		return ret;
> +	}
> +
> +	mutex_lock(&bd70528->rtc_timer_lock);
> +	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WDT_CTRL, &reg);
> +	mutex_unlock(&bd70528->rtc_timer_lock);
> +

I don't see the point for the above mutex locks. This is just reading a
single register. regmap itself provides locking for that already.

> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to get the watchdog state\n");
> +		return ret;
> +	}
> +	if (reg & BD70528_MASK_WDT_EN) {
> +		dev_dbg(&pdev->dev, "watchdog was running during probe\n");
> +		set_bit(WDOG_HW_RUNNING, &wdt->status);
> +	}
> +
> +	ret = devm_watchdog_register_device(&pdev->dev, wdt);
> +	if (ret < 0)
> +		dev_err(&pdev->dev, "watchdog registration failed: %d\n", ret);
> +
> +	return ret;
> +}
> +static struct platform_driver bd70528_wdt = {
> +	.driver = {
> +		.name = "bd70528-wdt"
> +	},
> +	.probe = bd70528_wdt_probe,
> +};
> +
> +module_platform_driver(bd70528_wdt);
> +
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("BD70528 watchdog driver");
> +MODULE_LICENSE("GPL");
> 

Re: [RFC PATCH v2 10/10] watchdog: bd70528: Initial support for ROHM BD70528 watchdog block

[Matti Vaittinen]

On Sat, Jan 26, 2019 at 08:36:14AM -0800, Guenter Roeck wrote:
> On 1/25/19 3:06 AM, Matti Vaittinen wrote:
> > +/* Max time we can set is 1 hour, 59 minutes and 59 seconds */
> > +#define WDT_MAX_MS ((2 * 60 * 60 - 1) * 1000)
> > +/* Minimum time is 1 second */
> > +#define WDT_MIN_MS 1000
> > +#define DEFAULT_TIMEOUT 60
> > +
> > +static int bd70528_wdt_probe(struct platform_device *pdev)
> > +{
> > +	struct bd70528 *bd70528;
> > +	struct wdtbd70528 *w;
> > +	int ret;
> > +	unsigned int reg;
> > +	struct watchdog_device *wdt;
> > +
> > +	bd70528 = dev_get_drvdata(pdev->dev.parent);
> > +	if (!bd70528) {
> > +		dev_err(&pdev->dev, "No MFD driver data\n");
> > +		return -EINVAL;
> > +	}
> > +	w = devm_kzalloc(&pdev->dev, sizeof(*w), GFP_KERNEL);
> > +	if (!w)
> > +		return -ENOMEM;
> > +	w->bd = bd70528;
> 
> Unless I am missing something, only the mutex and the regmap pointer
> are used from struct bd70528. With that in mind, it might be desirable
> to copy those pointers to struct wdtbd70528 to avoid the additional
> dereferencing at runtime.

You are not missing anyting. If we ever add support for another PMIC
variant we will probably be using also the chip_type. Untill then only
the mutex and regmap. (And maybe the of_node if we have any RTC
properties in DT). So at this point we just use regmap and mutex. I will
change this.

> > +	w->dev = &pdev->dev;
> > +
> > +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> > +	if (!wdt)
> > +		return -ENOMEM;
> > +
> 
> struct watchdog_device can be part of struct wdtbd70528. Two separate allocations
> are not necessary.

Correct. Thanks for pointing that out. I'll simplify this.

> > +	wdt->info = &bd70528_wdt_info;
> > +	wdt->ops =  &bd70528_wdt_ops;
> > +	wdt->min_hw_heartbeat_ms = WDT_MIN_MS;
> > +	wdt->max_hw_heartbeat_ms = WDT_MAX_MS;
> > +	wdt->parent = pdev->dev.parent;
> > +	wdt->timeout = DEFAULT_TIMEOUT;
> > +	watchdog_set_drvdata(wdt, w);
> > +	watchdog_init_timeout(wdt, 0, pdev->dev.parent);
> > +
> > +	ret = bd70528_wdt_set_timeout(wdt, wdt->timeout);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to set the watchdog timeout\n");
> > +		return ret;
> > +	}
> > +
> > +	mutex_lock(&bd70528->rtc_timer_lock);
> > +	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WDT_CTRL, &reg);
> > +	mutex_unlock(&bd70528->rtc_timer_lock);
> > +
> 
> I don't see the point for the above mutex locks. This is just reading a
> single register. regmap itself provides locking for that already.

It has a purpose here - but I'd better add a comment. We want to get the
initial state of WDG here. If the probe is executed when RTC time is being
set we may read the state just when RTC is (temporarily) disabling WDT -
and we might tell the WDT core that WDT is disabled - even if it is
actually enabled. The mutex prevents us from reading the WDT state when
RTC is being set.
 
Br,
	Matti Vaittinen

Re: [RFC PATCH v2 10/10] watchdog: bd70528: Initial support for ROHM BD70528 watchdog block

[Matti Vaittinen]

On Mon, Jan 28, 2019 at 10:00:35AM +0200, Matti Vaittinen wrote:
> On Sat, Jan 26, 2019 at 08:36:14AM -0800, Guenter Roeck wrote:
> > On 1/25/19 3:06 AM, Matti Vaittinen wrote:
> > > +/* Max time we can set is 1 hour, 59 minutes and 59 seconds */
> > > +#define WDT_MAX_MS ((2 * 60 * 60 - 1) * 1000)
> > > +/* Minimum time is 1 second */
> > > +#define WDT_MIN_MS 1000
> > > +#define DEFAULT_TIMEOUT 60
> > > +
> > > +static int bd70528_wdt_probe(struct platform_device *pdev)
> > > +{
> > > +	struct bd70528 *bd70528;
> > > +	struct wdtbd70528 *w;
> > > +	int ret;
> > > +	unsigned int reg;
> > > +	struct watchdog_device *wdt;
> > > +
> > > +	bd70528 = dev_get_drvdata(pdev->dev.parent);
> > > +	if (!bd70528) {
> > > +		dev_err(&pdev->dev, "No MFD driver data\n");
> > > +		return -EINVAL;
> > > +	}
> > > +	w = devm_kzalloc(&pdev->dev, sizeof(*w), GFP_KERNEL);
> > > +	if (!w)
> > > +		return -ENOMEM;
> > > +	w->bd = bd70528;
> > 
> > Unless I am missing something, only the mutex and the regmap pointer
> > are used from struct bd70528. With that in mind, it might be desirable
> > to copy those pointers to struct wdtbd70528 to avoid the additional
> > dereferencing at runtime.
> 
> You are not missing anyting. If we ever add support for another PMIC
> variant we will probably be using also the chip_type. Untill then only
> the mutex and regmap. (And maybe the of_node if we have any RTC
> properties in DT). So at this point we just use regmap and mutex. I will
> change this.

Oh, but actually we are also using the WDT state setting function
provided by MFD. And this is taking the struct bd70528 as parameter. So
maybe we can keep it like this afterall? 

> 
> > > +	w->dev = &pdev->dev;
> > > +
> > > +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> > > +	if (!wdt)
> > > +		return -ENOMEM;
> > > +
> > 
> > struct watchdog_device can be part of struct wdtbd70528. Two separate allocations
> > are not necessary.
> 
> Correct. Thanks for pointing that out. I'll simplify this.
> 
> > > +	wdt->info = &bd70528_wdt_info;
> > > +	wdt->ops =  &bd70528_wdt_ops;
> > > +	wdt->min_hw_heartbeat_ms = WDT_MIN_MS;
> > > +	wdt->max_hw_heartbeat_ms = WDT_MAX_MS;
> > > +	wdt->parent = pdev->dev.parent;
> > > +	wdt->timeout = DEFAULT_TIMEOUT;
> > > +	watchdog_set_drvdata(wdt, w);
> > > +	watchdog_init_timeout(wdt, 0, pdev->dev.parent);
> > > +
> > > +	ret = bd70528_wdt_set_timeout(wdt, wdt->timeout);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "Failed to set the watchdog timeout\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	mutex_lock(&bd70528->rtc_timer_lock);
> > > +	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WDT_CTRL, &reg);
> > > +	mutex_unlock(&bd70528->rtc_timer_lock);
> > > +
> > 
> > I don't see the point for the above mutex locks. This is just reading a
> > single register. regmap itself provides locking for that already.
> 
> It has a purpose here - but I'd better add a comment. We want to get the
> initial state of WDG here. If the probe is executed when RTC time is being
> set we may read the state just when RTC is (temporarily) disabling WDT -
> and we might tell the WDT core that WDT is disabled - even if it is
> actually enabled. The mutex prevents us from reading the WDT state when
> RTC is being set.
>  
> Br,
> 	Matti Vaittinen

Re: [RFC PATCH v2 10/10] watchdog: bd70528: Initial support for ROHM BD70528 watchdog block

[Guenter Roeck]

On Mon, Jan 28, 2019 at 10:13:47AM +0200, Matti Vaittinen wrote:
> On Mon, Jan 28, 2019 at 10:00:35AM +0200, Matti Vaittinen wrote:
> > On Sat, Jan 26, 2019 at 08:36:14AM -0800, Guenter Roeck wrote:
> > > On 1/25/19 3:06 AM, Matti Vaittinen wrote:
> > > > +/* Max time we can set is 1 hour, 59 minutes and 59 seconds */
> > > > +#define WDT_MAX_MS ((2 * 60 * 60 - 1) * 1000)
> > > > +/* Minimum time is 1 second */
> > > > +#define WDT_MIN_MS 1000
> > > > +#define DEFAULT_TIMEOUT 60
> > > > +
> > > > +static int bd70528_wdt_probe(struct platform_device *pdev)
> > > > +{
> > > > +	struct bd70528 *bd70528;
> > > > +	struct wdtbd70528 *w;
> > > > +	int ret;
> > > > +	unsigned int reg;
> > > > +	struct watchdog_device *wdt;
> > > > +
> > > > +	bd70528 = dev_get_drvdata(pdev->dev.parent);
> > > > +	if (!bd70528) {
> > > > +		dev_err(&pdev->dev, "No MFD driver data\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +	w = devm_kzalloc(&pdev->dev, sizeof(*w), GFP_KERNEL);
> > > > +	if (!w)
> > > > +		return -ENOMEM;
> > > > +	w->bd = bd70528;
> > > 
> > > Unless I am missing something, only the mutex and the regmap pointer
> > > are used from struct bd70528. With that in mind, it might be desirable
> > > to copy those pointers to struct wdtbd70528 to avoid the additional
> > > dereferencing at runtime.
> > 
> > You are not missing anyting. If we ever add support for another PMIC
> > variant we will probably be using also the chip_type. Untill then only
> > the mutex and regmap. (And maybe the of_node if we have any RTC
> > properties in DT). So at this point we just use regmap and mutex. I will
> > change this.
> 
> Oh, but actually we are also using the WDT state setting function
> provided by MFD. And this is taking the struct bd70528 as parameter. So
> maybe we can keep it like this afterall? 
> 
Ok.

Guenter

> > 
> > > > +	w->dev = &pdev->dev;
> > > > +
> > > > +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> > > > +	if (!wdt)
> > > > +		return -ENOMEM;
> > > > +
> > > 
> > > struct watchdog_device can be part of struct wdtbd70528. Two separate allocations
> > > are not necessary.
> > 
> > Correct. Thanks for pointing that out. I'll simplify this.
> > 
> > > > +	wdt->info = &bd70528_wdt_info;
> > > > +	wdt->ops =  &bd70528_wdt_ops;
> > > > +	wdt->min_hw_heartbeat_ms = WDT_MIN_MS;
> > > > +	wdt->max_hw_heartbeat_ms = WDT_MAX_MS;
> > > > +	wdt->parent = pdev->dev.parent;
> > > > +	wdt->timeout = DEFAULT_TIMEOUT;
> > > > +	watchdog_set_drvdata(wdt, w);
> > > > +	watchdog_init_timeout(wdt, 0, pdev->dev.parent);
> > > > +
> > > > +	ret = bd70528_wdt_set_timeout(wdt, wdt->timeout);
> > > > +	if (ret) {
> > > > +		dev_err(&pdev->dev, "Failed to set the watchdog timeout\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	mutex_lock(&bd70528->rtc_timer_lock);
> > > > +	ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WDT_CTRL, &reg);
> > > > +	mutex_unlock(&bd70528->rtc_timer_lock);
> > > > +
> > > 
> > > I don't see the point for the above mutex locks. This is just reading a
> > > single register. regmap itself provides locking for that already.
> > 
> > It has a purpose here - but I'd better add a comment. We want to get the
> > initial state of WDG here. If the probe is executed when RTC time is being
> > set we may read the state just when RTC is (temporarily) disabling WDT -
> > and we might tell the WDT core that WDT is disabled - even if it is
> > actually enabled. The mutex prevents us from reading the WDT state when
> > RTC is being set.
> >  
> > Br,
> > 	Matti Vaittinen