From: Ludovic Barre <[email protected]>
This patch series updates stm32 watchdog driver on:
-use devm_watchdog_register_device
-Guenter's recommendation about return value:
set_timeout return 0 on succes, -EINVAL for "parameter out of range"
and -EIO for "could not write value to the watchdog".
Set of reload and prescaler registers are stay in start function,
because the stm32 watchdog must be enabled to write these registers.
-adds dynamic prescaler support
Ludovic Barre (3):
watchdog: stm32: update to devm_watchdog_register_device
watchdog: stm32: update return values recommended by watchdog kernel
api
watchdog: stm32: add dynamic prescaler support
drivers/watchdog/stm32_iwdg.c | 96 ++++++++++++++++++++++++-------------------
1 file changed, 54 insertions(+), 42 deletions(-)
--
2.7.4
From: Ludovic Barre <[email protected]>
This patch updates return values on watchdog-kernel-api.txt:
return 0 on succes, -EINVAL for "parameter out of range"
and -EIO for "could not write value to the watchdog".
Signed-off-by: Ludovic Barre <[email protected]>
---
drivers/watchdog/stm32_iwdg.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c
index e191bd8..f19a6d4 100644
--- a/drivers/watchdog/stm32_iwdg.c
+++ b/drivers/watchdog/stm32_iwdg.c
@@ -81,7 +81,6 @@ static int stm32_iwdg_start(struct watchdog_device *wdd)
struct stm32_iwdg *wdt = watchdog_get_drvdata(wdd);
u32 val = FLAG_PVU | FLAG_RVU;
u32 reload;
- int ret;
dev_dbg(wdd->parent, "%s\n", __func__);
@@ -98,13 +97,11 @@ static int stm32_iwdg_start(struct watchdog_device *wdd)
reg_write(wdt->regs, IWDG_KR, KR_KEY_ENABLE);
/* wait for the registers to be updated (max 100ms) */
- ret = readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, val,
- !(val & (FLAG_PVU | FLAG_RVU)),
- SLEEP_US, TIMEOUT_US);
- if (ret) {
- dev_err(wdd->parent,
- "Fail to set prescaler or reload registers\n");
- return ret;
+ if (readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, val,
+ !(val & (FLAG_PVU | FLAG_RVU)),
+ SLEEP_US, TIMEOUT_US)) {
+ dev_err(wdd->parent, "Fail to set prescaler, reload regs\n");
+ return -EIO;
}
/* reload watchdog */
@@ -128,8 +125,16 @@ static int stm32_iwdg_ping(struct watchdog_device *wdd)
static int stm32_iwdg_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)
{
+ unsigned int tout = clamp(timeout, wdd->min_timeout,
+ wdd->max_hw_heartbeat_ms / 1000);
+
dev_dbg(wdd->parent, "%s timeout: %d sec\n", __func__, timeout);
+ if (tout != timeout) {
+ dev_err(wdd->parent, "parameter out of range\n");
+ return -EINVAL;
+ }
+
wdd->timeout = timeout;
if (watchdog_active(wdd))
--
2.7.4
From: Ludovic Barre <[email protected]>
This patch updates to devm_watchdog_register_device interface
Signed-off-by: Ludovic Barre <[email protected]>
---
drivers/watchdog/stm32_iwdg.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c
index e00e3b3..e191bd8 100644
--- a/drivers/watchdog/stm32_iwdg.c
+++ b/drivers/watchdog/stm32_iwdg.c
@@ -243,7 +243,7 @@ static int stm32_iwdg_probe(struct platform_device *pdev)
dev_warn(&pdev->dev,
"unable to set timeout value, using default\n");
- ret = watchdog_register_device(wdd);
+ ret = devm_watchdog_register_device(&pdev->dev, wdd);
if (ret) {
dev_err(&pdev->dev, "failed to register watchdog device\n");
goto err;
@@ -263,7 +263,6 @@ static int stm32_iwdg_remove(struct platform_device *pdev)
{
struct stm32_iwdg *wdt = platform_get_drvdata(pdev);
- watchdog_unregister_device(&wdt->wdd);
clk_disable_unprepare(wdt->clk_lsi);
clk_disable_unprepare(wdt->clk_pclk);
--
2.7.4
From: Ludovic Barre <[email protected]>
This patch allows to define the max prescaler by compatible.
To set a large range of timeout, the prescaler should be set
dynamically (from the timeout request) to improve the resolution
in order to have a timeout close to the expected value.
Signed-off-by: Ludovic Barre <[email protected]>
---
drivers/watchdog/stm32_iwdg.c | 76 ++++++++++++++++++++++++-------------------
1 file changed, 42 insertions(+), 34 deletions(-)
diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c
index f19a6d4..0c765d4 100644
--- a/drivers/watchdog/stm32_iwdg.c
+++ b/drivers/watchdog/stm32_iwdg.c
@@ -34,18 +34,12 @@
#define KR_KEY_EWA 0x5555 /* write access enable */
#define KR_KEY_DWA 0x0000 /* write access disable */
-/* IWDG_PR register bit values */
-#define PR_4 0x00 /* prescaler set to 4 */
-#define PR_8 0x01 /* prescaler set to 8 */
-#define PR_16 0x02 /* prescaler set to 16 */
-#define PR_32 0x03 /* prescaler set to 32 */
-#define PR_64 0x04 /* prescaler set to 64 */
-#define PR_128 0x05 /* prescaler set to 128 */
-#define PR_256 0x06 /* prescaler set to 256 */
+#define PR_SHIFT 2
+#define PR_MIN BIT(PR_SHIFT)
/* IWDG_RLR register values */
-#define RLR_MIN 0x07C /* min value supported by reload register */
-#define RLR_MAX 0xFFF /* max value supported by reload register */
+#define RLR_MIN 0x2 /* min value recommended */
+#define RLR_MAX GENMASK(11, 0) /* max value of reload register */
/* IWDG_SR register bit mask */
#define FLAG_PVU BIT(0) /* Watchdog prescaler value update */
@@ -55,15 +49,28 @@
#define TIMEOUT_US 100000
#define SLEEP_US 1000
-#define HAS_PCLK true
+struct stm32_iwdg_data {
+ bool has_pclk;
+ u32 max_prescaler;
+};
+
+static const struct stm32_iwdg_data stm32_iwdg_data = {
+ .has_pclk = false,
+ .max_prescaler = 256,
+};
+
+static const struct stm32_iwdg_data stm32mp1_iwdg_data = {
+ .has_pclk = true,
+ .max_prescaler = 1024,
+};
struct stm32_iwdg {
struct watchdog_device wdd;
+ const struct stm32_iwdg_data *data;
void __iomem *regs;
struct clk *clk_lsi;
struct clk *clk_pclk;
unsigned int rate;
- bool has_pclk;
};
static inline u32 reg_read(void __iomem *base, u32 reg)
@@ -79,26 +86,28 @@ static inline void reg_write(void __iomem *base, u32 reg, u32 val)
static int stm32_iwdg_start(struct watchdog_device *wdd)
{
struct stm32_iwdg *wdt = watchdog_get_drvdata(wdd);
- u32 val = FLAG_PVU | FLAG_RVU;
- u32 reload;
+ u32 presc, iwdg_rlr, iwdg_pr, iwdg_sr;
dev_dbg(wdd->parent, "%s\n", __func__);
- /* prescaler fixed to 256 */
- reload = clamp_t(unsigned int, ((wdd->timeout * wdt->rate) / 256) - 1,
- RLR_MIN, RLR_MAX);
+ presc = DIV_ROUND_UP(wdd->timeout * wdt->rate, RLR_MAX + 1);
+
+ /* The prescaler is align on power of 2 and start at 2 ^ PR_SHIFT. */
+ presc = roundup_pow_of_two(presc);
+ iwdg_pr = presc <= 1 << PR_SHIFT ? 0 : ilog2(presc) - PR_SHIFT;
+ iwdg_rlr = ((wdd->timeout * wdt->rate) / presc) - 1;
+ /* enable watchdog */
+ reg_write(wdt->regs, IWDG_KR, KR_KEY_ENABLE);
/* enable write access */
reg_write(wdt->regs, IWDG_KR, KR_KEY_EWA);
-
/* set prescaler & reload registers */
- reg_write(wdt->regs, IWDG_PR, PR_256); /* prescaler fix to 256 */
- reg_write(wdt->regs, IWDG_RLR, reload);
- reg_write(wdt->regs, IWDG_KR, KR_KEY_ENABLE);
+ reg_write(wdt->regs, IWDG_PR, iwdg_pr);
+ reg_write(wdt->regs, IWDG_RLR, iwdg_rlr);
/* wait for the registers to be updated (max 100ms) */
- if (readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, val,
- !(val & (FLAG_PVU | FLAG_RVU)),
+ if (readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, iwdg_sr,
+ !(iwdg_sr & (FLAG_PVU | FLAG_RVU)),
SLEEP_US, TIMEOUT_US)) {
dev_err(wdd->parent, "Fail to set prescaler, reload regs\n");
return -EIO;
@@ -155,7 +164,7 @@ static int stm32_iwdg_clk_init(struct platform_device *pdev,
}
/* optional peripheral clock */
- if (wdt->has_pclk) {
+ if (wdt->data->has_pclk) {
wdt->clk_pclk = devm_clk_get(&pdev->dev, "pclk");
if (IS_ERR(wdt->clk_pclk)) {
dev_err(&pdev->dev, "Unable to get pclk clock\n");
@@ -196,8 +205,8 @@ static const struct watchdog_ops stm32_iwdg_ops = {
};
static const struct of_device_id stm32_iwdg_of_match[] = {
- { .compatible = "st,stm32-iwdg", .data = (void *)!HAS_PCLK },
- { .compatible = "st,stm32mp1-iwdg", .data = (void *)HAS_PCLK },
+ { .compatible = "st,stm32-iwdg", .data = &stm32_iwdg_data },
+ { .compatible = "st,stm32mp1-iwdg", .data = &stm32mp1_iwdg_data },
{ /* end node */ }
};
MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match);
@@ -205,20 +214,17 @@ MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match);
static int stm32_iwdg_probe(struct platform_device *pdev)
{
struct watchdog_device *wdd;
- const struct of_device_id *match;
struct stm32_iwdg *wdt;
struct resource *res;
int ret;
- match = of_match_device(stm32_iwdg_of_match, &pdev->dev);
- if (!match)
- return -ENODEV;
-
wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
if (!wdt)
return -ENOMEM;
- wdt->has_pclk = match->data;
+ wdt->data = of_device_get_match_data(&pdev->dev);
+ if (!wdt->data)
+ return -ENODEV;
/* This is the timer base. */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -236,8 +242,10 @@ static int stm32_iwdg_probe(struct platform_device *pdev)
wdd = &wdt->wdd;
wdd->info = &stm32_iwdg_info;
wdd->ops = &stm32_iwdg_ops;
- wdd->min_timeout = ((RLR_MIN + 1) * 256) / wdt->rate;
- wdd->max_hw_heartbeat_ms = ((RLR_MAX + 1) * 256 * 1000) / wdt->rate;
+ wdd->min_timeout = max_t(unsigned int, 1,
+ (((RLR_MIN + 1) * PR_MIN) / wdt->rate));
+ wdd->max_hw_heartbeat_ms = ((RLR_MAX + 1) * wdt->data->max_prescaler *
+ 1000) / wdt->rate;
wdd->parent = &pdev->dev;
watchdog_set_drvdata(wdd, wdt);
--
2.7.4
On Thu, May 02, 2019 at 04:08:44PM +0200, Ludovic Barre wrote:
> From: Ludovic Barre <[email protected]>
>
> This patch updates to devm_watchdog_register_device interface
>
Not that easy. See below.
A more complete solution is at
https://patchwork.kernel.org/patch/10894355
I have a total of three patches for this driver pending for
the next kernel release. Maybe it would make sense to (re-)
start this series from there after the next commit window
closes.
Guenter
> Signed-off-by: Ludovic Barre <[email protected]>
> ---
> drivers/watchdog/stm32_iwdg.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c
> index e00e3b3..e191bd8 100644
> --- a/drivers/watchdog/stm32_iwdg.c
> +++ b/drivers/watchdog/stm32_iwdg.c
> @@ -243,7 +243,7 @@ static int stm32_iwdg_probe(struct platform_device *pdev)
> dev_warn(&pdev->dev,
> "unable to set timeout value, using default\n");
>
> - ret = watchdog_register_device(wdd);
> + ret = devm_watchdog_register_device(&pdev->dev, wdd);
> if (ret) {
> dev_err(&pdev->dev, "failed to register watchdog device\n");
> goto err;
> @@ -263,7 +263,6 @@ static int stm32_iwdg_remove(struct platform_device *pdev)
> {
> struct stm32_iwdg *wdt = platform_get_drvdata(pdev);
>
> - watchdog_unregister_device(&wdt->wdd);
> clk_disable_unprepare(wdt->clk_lsi);
> clk_disable_unprepare(wdt->clk_pclk);
This disables the clock while the watchdog is still registered
and running. That is not a good idea.
>
> --
> 2.7.4
>
On Thu, May 02, 2019 at 04:08:45PM +0200, Ludovic Barre wrote:
> From: Ludovic Barre <[email protected]>
>
> This patch updates return values on watchdog-kernel-api.txt:
> return 0 on succes, -EINVAL for "parameter out of range"
> and -EIO for "could not write value to the watchdog".
>
> Signed-off-by: Ludovic Barre <[email protected]>
> ---
> drivers/watchdog/stm32_iwdg.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c
> index e191bd8..f19a6d4 100644
> --- a/drivers/watchdog/stm32_iwdg.c
> +++ b/drivers/watchdog/stm32_iwdg.c
> @@ -81,7 +81,6 @@ static int stm32_iwdg_start(struct watchdog_device *wdd)
> struct stm32_iwdg *wdt = watchdog_get_drvdata(wdd);
> u32 val = FLAG_PVU | FLAG_RVU;
> u32 reload;
> - int ret;
>
> dev_dbg(wdd->parent, "%s\n", __func__);
>
> @@ -98,13 +97,11 @@ static int stm32_iwdg_start(struct watchdog_device *wdd)
> reg_write(wdt->regs, IWDG_KR, KR_KEY_ENABLE);
>
> /* wait for the registers to be updated (max 100ms) */
> - ret = readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, val,
> - !(val & (FLAG_PVU | FLAG_RVU)),
> - SLEEP_US, TIMEOUT_US);
> - if (ret) {
> - dev_err(wdd->parent,
> - "Fail to set prescaler or reload registers\n");
> - return ret;
> + if (readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, val,
> + !(val & (FLAG_PVU | FLAG_RVU)),
> + SLEEP_US, TIMEOUT_US)) {
> + dev_err(wdd->parent, "Fail to set prescaler, reload regs\n");
> + return -EIO;
Please don't. Overriding error values tends to result in complaints by
static analyzers, and we don't want to have to deal with those.
> }
>
> /* reload watchdog */
> @@ -128,8 +125,16 @@ static int stm32_iwdg_ping(struct watchdog_device *wdd)
> static int stm32_iwdg_set_timeout(struct watchdog_device *wdd,
> unsigned int timeout)
> {
> + unsigned int tout = clamp(timeout, wdd->min_timeout,
> + wdd->max_hw_heartbeat_ms / 1000);
> +
> dev_dbg(wdd->parent, "%s timeout: %d sec\n", __func__, timeout);
>
> + if (tout != timeout) {
> + dev_err(wdd->parent, "parameter out of range\n");
> + return -EINVAL;
> + }
This is simply wrong. The whole point of max_hw_heartbeat_ms is to
enable situations where the selected timeout is larger than the
timeout supported by the hardware. In that situation, the kernel
pings the hardware until the next ping from userspace is due.
> +
> wdd->timeout = timeout;
>
> if (watchdog_active(wdd))
> --
> 2.7.4
>
On Thu, May 02, 2019 at 04:08:46PM +0200, Ludovic Barre wrote:
> From: Ludovic Barre <[email protected]>
>
> This patch allows to define the max prescaler by compatible.
> To set a large range of timeout, the prescaler should be set
> dynamically (from the timeout request) to improve the resolution
> in order to have a timeout close to the expected value.
>
> Signed-off-by: Ludovic Barre <[email protected]>
> ---
> drivers/watchdog/stm32_iwdg.c | 76 ++++++++++++++++++++++++-------------------
> 1 file changed, 42 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c
> index f19a6d4..0c765d4 100644
> --- a/drivers/watchdog/stm32_iwdg.c
> +++ b/drivers/watchdog/stm32_iwdg.c
> @@ -34,18 +34,12 @@
> #define KR_KEY_EWA 0x5555 /* write access enable */
> #define KR_KEY_DWA 0x0000 /* write access disable */
>
> -/* IWDG_PR register bit values */
> -#define PR_4 0x00 /* prescaler set to 4 */
> -#define PR_8 0x01 /* prescaler set to 8 */
> -#define PR_16 0x02 /* prescaler set to 16 */
> -#define PR_32 0x03 /* prescaler set to 32 */
> -#define PR_64 0x04 /* prescaler set to 64 */
> -#define PR_128 0x05 /* prescaler set to 128 */
> -#define PR_256 0x06 /* prescaler set to 256 */
> +#define PR_SHIFT 2
> +#define PR_MIN BIT(PR_SHIFT)
>
> /* IWDG_RLR register values */
> -#define RLR_MIN 0x07C /* min value supported by reload register */
> -#define RLR_MAX 0xFFF /* max value supported by reload register */
> +#define RLR_MIN 0x2 /* min value recommended */
> +#define RLR_MAX GENMASK(11, 0) /* max value of reload register */
>
> /* IWDG_SR register bit mask */
> #define FLAG_PVU BIT(0) /* Watchdog prescaler value update */
> @@ -55,15 +49,28 @@
> #define TIMEOUT_US 100000
> #define SLEEP_US 1000
>
> -#define HAS_PCLK true
> +struct stm32_iwdg_data {
> + bool has_pclk;
> + u32 max_prescaler;
> +};
> +
> +static const struct stm32_iwdg_data stm32_iwdg_data = {
> + .has_pclk = false,
> + .max_prescaler = 256,
> +};
> +
> +static const struct stm32_iwdg_data stm32mp1_iwdg_data = {
> + .has_pclk = true,
> + .max_prescaler = 1024,
> +};
>
> struct stm32_iwdg {
> struct watchdog_device wdd;
> + const struct stm32_iwdg_data *data;
> void __iomem *regs;
> struct clk *clk_lsi;
> struct clk *clk_pclk;
> unsigned int rate;
> - bool has_pclk;
> };
>
> static inline u32 reg_read(void __iomem *base, u32 reg)
> @@ -79,26 +86,28 @@ static inline void reg_write(void __iomem *base, u32 reg, u32 val)
> static int stm32_iwdg_start(struct watchdog_device *wdd)
> {
> struct stm32_iwdg *wdt = watchdog_get_drvdata(wdd);
> - u32 val = FLAG_PVU | FLAG_RVU;
> - u32 reload;
> + u32 presc, iwdg_rlr, iwdg_pr, iwdg_sr;
>
> dev_dbg(wdd->parent, "%s\n", __func__);
>
> - /* prescaler fixed to 256 */
> - reload = clamp_t(unsigned int, ((wdd->timeout * wdt->rate) / 256) - 1,
> - RLR_MIN, RLR_MAX);
> + presc = DIV_ROUND_UP(wdd->timeout * wdt->rate, RLR_MAX + 1);
> +
> + /* The prescaler is align on power of 2 and start at 2 ^ PR_SHIFT. */
> + presc = roundup_pow_of_two(presc);
> + iwdg_pr = presc <= 1 << PR_SHIFT ? 0 : ilog2(presc) - PR_SHIFT;
> + iwdg_rlr = ((wdd->timeout * wdt->rate) / presc) - 1;
>
> + /* enable watchdog */
> + reg_write(wdt->regs, IWDG_KR, KR_KEY_ENABLE);
> /* enable write access */
> reg_write(wdt->regs, IWDG_KR, KR_KEY_EWA);
> -
> /* set prescaler & reload registers */
> - reg_write(wdt->regs, IWDG_PR, PR_256); /* prescaler fix to 256 */
> - reg_write(wdt->regs, IWDG_RLR, reload);
> - reg_write(wdt->regs, IWDG_KR, KR_KEY_ENABLE);
> + reg_write(wdt->regs, IWDG_PR, iwdg_pr);
> + reg_write(wdt->regs, IWDG_RLR, iwdg_rlr);
>
> /* wait for the registers to be updated (max 100ms) */
> - if (readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, val,
> - !(val & (FLAG_PVU | FLAG_RVU)),
> + if (readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, iwdg_sr,
> + !(iwdg_sr & (FLAG_PVU | FLAG_RVU)),
> SLEEP_US, TIMEOUT_US)) {
> dev_err(wdd->parent, "Fail to set prescaler, reload regs\n");
> return -EIO;
As mentioned in the other patch, pelase return the error returned from
readl_relaxed_poll_timeout(). If that returns a timeout, we want to know
about it and not just hide it behind -EIO.
> @@ -155,7 +164,7 @@ static int stm32_iwdg_clk_init(struct platform_device *pdev,
> }
>
> /* optional peripheral clock */
> - if (wdt->has_pclk) {
> + if (wdt->data->has_pclk) {
> wdt->clk_pclk = devm_clk_get(&pdev->dev, "pclk");
> if (IS_ERR(wdt->clk_pclk)) {
> dev_err(&pdev->dev, "Unable to get pclk clock\n");
> @@ -196,8 +205,8 @@ static const struct watchdog_ops stm32_iwdg_ops = {
> };
>
> static const struct of_device_id stm32_iwdg_of_match[] = {
> - { .compatible = "st,stm32-iwdg", .data = (void *)!HAS_PCLK },
> - { .compatible = "st,stm32mp1-iwdg", .data = (void *)HAS_PCLK },
> + { .compatible = "st,stm32-iwdg", .data = &stm32_iwdg_data },
> + { .compatible = "st,stm32mp1-iwdg", .data = &stm32mp1_iwdg_data },
> { /* end node */ }
> };
> MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match);
> @@ -205,20 +214,17 @@ MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match);
> static int stm32_iwdg_probe(struct platform_device *pdev)
> {
> struct watchdog_device *wdd;
> - const struct of_device_id *match;
> struct stm32_iwdg *wdt;
> struct resource *res;
> int ret;
>
> - match = of_match_device(stm32_iwdg_of_match, &pdev->dev);
> - if (!match)
> - return -ENODEV;
> -
> wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> if (!wdt)
> return -ENOMEM;
>
> - wdt->has_pclk = match->data;
> + wdt->data = of_device_get_match_data(&pdev->dev);
> + if (!wdt->data)
> + return -ENODEV;
>
> /* This is the timer base. */
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -236,8 +242,10 @@ static int stm32_iwdg_probe(struct platform_device *pdev)
> wdd = &wdt->wdd;
> wdd->info = &stm32_iwdg_info;
> wdd->ops = &stm32_iwdg_ops;
> - wdd->min_timeout = ((RLR_MIN + 1) * 256) / wdt->rate;
> - wdd->max_hw_heartbeat_ms = ((RLR_MAX + 1) * 256 * 1000) / wdt->rate;
> + wdd->min_timeout = max_t(unsigned int, 1,
> + (((RLR_MIN + 1) * PR_MIN) / wdt->rate));
Is that any different to DIV_ROUND_UP() ?
> + wdd->max_hw_heartbeat_ms = ((RLR_MAX + 1) * wdt->data->max_prescaler *
> + 1000) / wdt->rate;
> wdd->parent = &pdev->dev;
>
> watchdog_set_drvdata(wdd, wdt);
> --
> 2.7.4
>
On Thu, May 02, 2019 at 04:08:43PM +0200, Ludovic Barre wrote:
> From: Ludovic Barre <[email protected]>
>
> This patch series updates stm32 watchdog driver on:
> -use devm_watchdog_register_device
> -Guenter's recommendation about return value:
> set_timeout return 0 on succes, -EINVAL for "parameter out of range"
> and -EIO for "could not write value to the watchdog".
No, sorry, that is not what I meant.
set_timeout() should set ->timeout either to the requested value
if equal to or larger than the maximum timeout supported by the
hardware, and to the actually selected timeout otherwise.
Guenter
> Set of reload and prescaler registers are stay in start function,
> because the stm32 watchdog must be enabled to write these registers.
> -adds dynamic prescaler support
>
> Ludovic Barre (3):
> watchdog: stm32: update to devm_watchdog_register_device
> watchdog: stm32: update return values recommended by watchdog kernel
> api
> watchdog: stm32: add dynamic prescaler support
>
> drivers/watchdog/stm32_iwdg.c | 96 ++++++++++++++++++++++++-------------------
> 1 file changed, 54 insertions(+), 42 deletions(-)
>
> --
> 2.7.4
>
hi Guenter
On 5/2/19 10:21 PM, Guenter Roeck wrote:
> On Thu, May 02, 2019 at 04:08:44PM +0200, Ludovic Barre wrote:
>> From: Ludovic Barre <[email protected]>
>>
>> This patch updates to devm_watchdog_register_device interface
>>
> Not that easy. See below.
>
> A more complete solution is at
> https://patchwork.kernel.org/patch/10894355
>
> I have a total of three patches for this driver pending for
> the next kernel release. Maybe it would make sense to (re-)
> start this series from there after the next commit window
> closes.
>
I used the repository defined in MAINTAINERS file
git://http://www.linux-watchdog.org/linux-watchdog.git
but there is no next branch.
Today, I see your kernel.org repository
https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git/
And I see your next branch, so I will use it.
Regards,
Ludo
> Guenter
>
>> Signed-off-by: Ludovic Barre <[email protected]>
>> ---
>> drivers/watchdog/stm32_iwdg.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c
>> index e00e3b3..e191bd8 100644
>> --- a/drivers/watchdog/stm32_iwdg.c
>> +++ b/drivers/watchdog/stm32_iwdg.c
>> @@ -243,7 +243,7 @@ static int stm32_iwdg_probe(struct platform_device *pdev)
>> dev_warn(&pdev->dev,
>> "unable to set timeout value, using default\n");
>>
>> - ret = watchdog_register_device(wdd);
>> + ret = devm_watchdog_register_device(&pdev->dev, wdd);
>> if (ret) {
>> dev_err(&pdev->dev, "failed to register watchdog device\n");
>> goto err;
>> @@ -263,7 +263,6 @@ static int stm32_iwdg_remove(struct platform_device *pdev)
>> {
>> struct stm32_iwdg *wdt = platform_get_drvdata(pdev);
>>
>> - watchdog_unregister_device(&wdt->wdd);
>> clk_disable_unprepare(wdt->clk_lsi);
>> clk_disable_unprepare(wdt->clk_pclk);
>
> This disables the clock while the watchdog is still registered
> and running. That is not a good idea.
>
>>
>> --
>> 2.7.4
>>