| Message ID | 1408192545-23987-1-git-send-email-mpa@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 16, 2014 at 02:35:45PM +0200, Markus Pargmann wrote: > From: Krystian Garbaciak <krystian.garbaciak@diasemi.com> > > This driver supports the watchdog device inside the DA906x PMIC. > > Signed-off-by: Krystian Garbaciak <krystian.garbaciak@diasemi.com> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > --- > Notes: > Changes in v4: > - Fixed indentation > - Fixed lock without unlock > - Check for parent device and device driver data in probe(). If not present, > this is an invalid prober initialization, return -EINVAL. > > Changes in v3: > - Cleanup error handling for timeout selection and setting > - Fix race condition due to late wdt drvdata setup > - Remove static struct watchdog_device. It is not initialized in the probe > function > - Use WATCHDOG_NOWAYOUT_INIT_STATUS for the status > - Remove 0 shift using DA9063_TWDSCALE_SHIFT > > drivers/watchdog/Kconfig | 7 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/da9063_wdt.c | 231 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 239 insertions(+) > create mode 100644 drivers/watchdog/da9063_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index c845527b503a..8d5c9b33552a 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -87,6 +87,13 @@ config DA9055_WATCHDOG > This driver can also be built as a module. If so, the module > will be called da9055_wdt. > > +config DA9063_WATCHDOG > + tristate "Dialog DA9063 Watchdog" > + depends on MFD_DA9063 > + select WATCHDOG_CORE > + help > + Support for the watchdog in the DA9063 PMIC. > + Please add a note such as above, listing the module name. > config GPIO_WATCHDOG > tristate "Watchdog device controlled through GPIO-line" > depends on OF_GPIO > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 7b8a91ed20e7..ce4a7632d863 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -172,6 +172,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o > # Architecture Independent > obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o > obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o > +obj-$(CONFIG_DA9063_WATCHDOG) += da9063_wdt.o > obj-$(CONFIG_GPIO_WATCHDOG) += gpio_wdt.o > obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o > obj-$(CONFIG_WM8350_WATCHDOG) += wm8350_wdt.o > diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c > new file mode 100644 > index 000000000000..a2d46bc55805 > --- /dev/null > +++ b/drivers/watchdog/da9063_wdt.c > @@ -0,0 +1,231 @@ > +/* > + * Watchdog driver for DA9063 PMICs. > + * > + * Copyright(c) 2012 Dialog Semiconductor Ltd. > + * > + * Author: Mariusz Wojtasik <mariusz.wojtasik@diasemi.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/watchdog.h> > +#include <linux/platform_device.h> > +#include <linux/uaccess.h> > +#include <linux/slab.h> > +#include <linux/delay.h> > +#include <linux/mfd/da9063/registers.h> > +#include <linux/mfd/da9063/core.h> > +#include <linux/regmap.h> > + > +/* > + * Watchdog selector to timeout in seconds. > + * 0: WDT disabled; > + * others: timeout = 2048 ms * 2^(TWDSCALE-1). > + */ > +static const int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 }; > +#define DA9063_TWDSCALE_DISABLE 0 > +#define DA9063_TWDSCALE_MIN 1 > +#define DA9063_TWDSCALE_MAX (ARRAY_SIZE(wdt_timeout) - 1) > +#define DA9063_WDT_MIN_TIMEOUT wdt_timeout[DA9063_TWDSCALE_MIN] > +#define DA9063_WDT_MAX_TIMEOUT wdt_timeout[DA9063_TWDSCALE_MAX] > +#define DA9063_WDG_TIMEOUT wdt_timeout[3] > + > +struct da9063_watchdog { > + struct da9063 *da9063; > + struct watchdog_device wdtdev; > + struct mutex lock; > +}; > + > +static int da9063_wdt_timeout_to_sel(int secs) > +{ > + int i; > + > + for (i = DA9063_TWDSCALE_MIN; i <= DA9063_TWDSCALE_MAX; i++) { When building with W=1, gcc complains about this line. Would be great if you can have a look and fix it. > + if (wdt_timeout[i] >= secs) > + return i; > + } > + > + return -EINVAL; > +} > + > +static int da9063_wdt_disable(struct da9063 *da9063) > +{ > + return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D, > + DA9063_TWDSCALE_MASK, > + DA9063_TWDSCALE_DISABLE); > +} > + > +static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval) > +{ > + return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D, > + DA9063_TWDSCALE_MASK, regval); > +} > + > +static int _da9063_wdt_kick(struct da9063 *da9063) > +{ > + return regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, > + DA9063_WATCHDOG); > +} > + > +static int da9063_wdt_start(struct watchdog_device *wdd) > +{ > + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); > + unsigned int selector; > + int ret; > + > + selector = da9063_wdt_timeout_to_sel(wdt->wdtdev.timeout); > + if (selector < 0) { selector has to be an int for this to work. Building the driver with W=1 reports it, btw. > + dev_err(wdt->da9063->dev, "Timeout out of range (timeout=%d)\n", > + wdt->wdtdev.timeout); > + return selector; > + } > + > + mutex_lock(&wdt->lock); > + ret = _da9063_wdt_set_timeout(wdt->da9063, selector); > + if (ret) { > + dev_err(wdt->da9063->dev, "Watchdog failed to start (err = %d)\n", > + ret); > + } > + mutex_unlock(&wdt->lock); > + > + return ret; > +} > + > +static int da9063_wdt_stop(struct watchdog_device *wdd) > +{ > + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); > + int ret; > + > + mutex_lock(&wdt->lock); > + ret = da9063_wdt_disable(wdt->da9063); > + if (ret) > + dev_alert(wdt->da9063->dev, "Watchdog failed to stop (err = %d)\n", > + ret); You have a number of places where the continuation line _on purpose_ does not align to te opening '('. I would understand it if your indentation rules were in any way consistent, but I don't see that either. Ok, I can see that you dislike the rule that continuation lines should be aligned to '(', but to misalign even when the '(' matches a tab position doesn't really make any sense. > + mutex_unlock(&wdt->lock); > + > + return ret; > +} > + > +static int da9063_wdt_ping(struct watchdog_device *wdd) > +{ > + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); > + int ret; > + > + mutex_lock(&wdt->lock); > + ret = _da9063_wdt_kick(wdt->da9063); > + if (ret) > + dev_alert(wdt->da9063->dev, "Failed to ping the watchdog (err = %d)\n", > + ret); > + mutex_unlock(&wdt->lock); > + > + return ret; > +} > + > +static int da9063_wdt_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); > + unsigned int selector; > + int ret; > + > + selector = da9063_wdt_timeout_to_sel(timeout); > + if (selector < 0) { selector must be an int for this to work. > + dev_err(wdt->da9063->dev, "Unsupported watchdog timeout, should be between 2 and 131\n"); Please use a continuation line here. > + return -EINVAL; > + } > + > + mutex_lock(&wdt->lock); > + > + ret = _da9063_wdt_set_timeout(wdt->da9063, selector); > + if (ret) > + dev_err(wdt->da9063->dev, "Failed to set watchdog timeout (err = %d)\n", > + ret); > + else > + wdd->timeout = wdt_timeout[selector]; > + > + mutex_unlock(&wdt->lock); > + > + return ret; > +} > + > +static const struct watchdog_info da9063_watchdog_info = { > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, > + .identity = "DA9063 Watchdog", > +}; > + > +static const struct watchdog_ops da9063_watchdog_ops = { > + .owner = THIS_MODULE, > + .start = da9063_wdt_start, > + .stop = da9063_wdt_stop, > + .ping = da9063_wdt_ping, > + .set_timeout = da9063_wdt_set_timeout, > +}; > + > +static int da9063_wdt_probe(struct platform_device *pdev) > +{ > + int ret; > + struct da9063 *da9063; > + struct da9063_watchdog *wdt; > + > + if (!pdev->dev.parent) > + return -EINVAL; > + > + da9063 = dev_get_drvdata(pdev->dev.parent); > + if (!da9063) > + return -EINVAL; > + > + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + > + wdt->da9063 = da9063; > + mutex_init(&wdt->lock); > + > + wdt->wdtdev.info = &da9063_watchdog_info; > + wdt->wdtdev.ops = &da9063_watchdog_ops; > + wdt->wdtdev.min_timeout = DA9063_WDT_MIN_TIMEOUT; > + wdt->wdtdev.max_timeout = DA9063_WDT_MAX_TIMEOUT; > + wdt->wdtdev.timeout = DA9063_WDG_TIMEOUT; > + > + wdt->wdtdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS; > + > + watchdog_set_drvdata(&wdt->wdtdev, wdt); > + dev_set_drvdata(&pdev->dev, wdt); > + > + ret = watchdog_register_device(&wdt->wdtdev); > + if (ret) { > + dev_err(da9063->dev, "da9063-watchdog registration failed (err = %d)", > + ret); > + return ret; > + } > + > + return 0; > +} > + > +static int da9063_wdt_remove(struct platform_device *pdev) > +{ > + struct da9063_watchdog *wdt = dev_get_drvdata(&pdev->dev); > + > + watchdog_unregister_device(&wdt->wdtdev); > + > + return 0; > +} > + > +static struct platform_driver da9063_wdt_driver = { > + .probe = da9063_wdt_probe, > + .remove = da9063_wdt_remove, > + .driver = { > + .name = DA9063_DRVNAME_WATCHDOG, > + }, > +}; > +module_platform_driver(da9063_wdt_driver); > + > +MODULE_AUTHOR("Mariusz Wojtasik <mariusz.wojtasik@diasemi.com>"); > +MODULE_DESCRIPTION("Watchdog driver for Dialog DA9063"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:" DA9063_DRVNAME_WATCHDOG);
Hi, On Wed, Aug 20, 2014 at 07:43:22AM -0700, Guenter Roeck wrote: > On Sat, Aug 16, 2014 at 02:35:45PM +0200, Markus Pargmann wrote: > > From: Krystian Garbaciak <krystian.garbaciak@diasemi.com> > > > > This driver supports the watchdog device inside the DA906x PMIC. > > > > Signed-off-by: Krystian Garbaciak <krystian.garbaciak@diasemi.com> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > > > --- > > Notes: > > Changes in v4: > > - Fixed indentation > > - Fixed lock without unlock > > - Check for parent device and device driver data in probe(). If not present, > > this is an invalid prober initialization, return -EINVAL. > > > > Changes in v3: > > - Cleanup error handling for timeout selection and setting > > - Fix race condition due to late wdt drvdata setup > > - Remove static struct watchdog_device. It is not initialized in the probe > > function > > - Use WATCHDOG_NOWAYOUT_INIT_STATUS for the status > > - Remove 0 shift using DA9063_TWDSCALE_SHIFT > > > > drivers/watchdog/Kconfig | 7 ++ > > drivers/watchdog/Makefile | 1 + > > drivers/watchdog/da9063_wdt.c | 231 ++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 239 insertions(+) > > create mode 100644 drivers/watchdog/da9063_wdt.c > > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > > index c845527b503a..8d5c9b33552a 100644 > > --- a/drivers/watchdog/Kconfig > > +++ b/drivers/watchdog/Kconfig > > @@ -87,6 +87,13 @@ config DA9055_WATCHDOG > > This driver can also be built as a module. If so, the module > > will be called da9055_wdt. > > > > +config DA9063_WATCHDOG > > + tristate "Dialog DA9063 Watchdog" > > + depends on MFD_DA9063 > > + select WATCHDOG_CORE > > + help > > + Support for the watchdog in the DA9063 PMIC. > > + > Please add a note such as above, listing the module name. Added such a note. > > > config GPIO_WATCHDOG > > tristate "Watchdog device controlled through GPIO-line" > > depends on OF_GPIO > > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > > index 7b8a91ed20e7..ce4a7632d863 100644 > > --- a/drivers/watchdog/Makefile > > +++ b/drivers/watchdog/Makefile > > @@ -172,6 +172,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o > > # Architecture Independent > > obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o > > obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o > > +obj-$(CONFIG_DA9063_WATCHDOG) += da9063_wdt.o > > obj-$(CONFIG_GPIO_WATCHDOG) += gpio_wdt.o > > obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o > > obj-$(CONFIG_WM8350_WATCHDOG) += wm8350_wdt.o > > diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c > > new file mode 100644 > > index 000000000000..a2d46bc55805 > > --- /dev/null > > +++ b/drivers/watchdog/da9063_wdt.c > > @@ -0,0 +1,231 @@ > > +/* > > + * Watchdog driver for DA9063 PMICs. > > + * > > + * Copyright(c) 2012 Dialog Semiconductor Ltd. > > + * > > + * Author: Mariusz Wojtasik <mariusz.wojtasik@diasemi.com> > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License as published by the > > + * Free Software Foundation; either version 2 of the License, or (at your > > + * option) any later version. > > + */ > > + > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/watchdog.h> > > +#include <linux/platform_device.h> > > +#include <linux/uaccess.h> > > +#include <linux/slab.h> > > +#include <linux/delay.h> > > +#include <linux/mfd/da9063/registers.h> > > +#include <linux/mfd/da9063/core.h> > > +#include <linux/regmap.h> > > + > > +/* > > + * Watchdog selector to timeout in seconds. > > + * 0: WDT disabled; > > + * others: timeout = 2048 ms * 2^(TWDSCALE-1). > > + */ > > +static const int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 }; > > +#define DA9063_TWDSCALE_DISABLE 0 > > +#define DA9063_TWDSCALE_MIN 1 > > +#define DA9063_TWDSCALE_MAX (ARRAY_SIZE(wdt_timeout) - 1) > > +#define DA9063_WDT_MIN_TIMEOUT wdt_timeout[DA9063_TWDSCALE_MIN] > > +#define DA9063_WDT_MAX_TIMEOUT wdt_timeout[DA9063_TWDSCALE_MAX] > > +#define DA9063_WDG_TIMEOUT wdt_timeout[3] > > + > > +struct da9063_watchdog { > > + struct da9063 *da9063; > > + struct watchdog_device wdtdev; > > + struct mutex lock; > > +}; > > + > > +static int da9063_wdt_timeout_to_sel(int secs) > > +{ > > + int i; > > + > > + for (i = DA9063_TWDSCALE_MIN; i <= DA9063_TWDSCALE_MAX; i++) { > > When building with W=1, gcc complains about this line. Would be great if you can > have a look and fix it. I just compiled with W=1, it doesn't complain here about this line. Could you give me the warning? > > > + if (wdt_timeout[i] >= secs) > > + return i; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int da9063_wdt_disable(struct da9063 *da9063) > > +{ > > + return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D, > > + DA9063_TWDSCALE_MASK, > > + DA9063_TWDSCALE_DISABLE); > > +} > > + > > +static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval) > > +{ > > + return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D, > > + DA9063_TWDSCALE_MASK, regval); > > +} > > + > > +static int _da9063_wdt_kick(struct da9063 *da9063) > > +{ > > + return regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, > > + DA9063_WATCHDOG); > > +} > > + > > +static int da9063_wdt_start(struct watchdog_device *wdd) > > +{ > > + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); > > + unsigned int selector; > > + int ret; > > + > > + selector = da9063_wdt_timeout_to_sel(wdt->wdtdev.timeout); > > + if (selector < 0) { > > selector has to be an int for this to work. Building the driver with W=1 reports > it, btw. Yes, thanks, I changed both selector variables to int. > > > + dev_err(wdt->da9063->dev, "Timeout out of range (timeout=%d)\n", > > + wdt->wdtdev.timeout); > > + return selector; > > + } > > + > > + mutex_lock(&wdt->lock); > > + ret = _da9063_wdt_set_timeout(wdt->da9063, selector); > > + if (ret) { > > + dev_err(wdt->da9063->dev, "Watchdog failed to start (err = %d)\n", > > + ret); > > + } > > + mutex_unlock(&wdt->lock); > > + > > + return ret; > > +} > > + > > +static int da9063_wdt_stop(struct watchdog_device *wdd) > > +{ > > + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); > > + int ret; > > + > > + mutex_lock(&wdt->lock); > > + ret = da9063_wdt_disable(wdt->da9063); > > + if (ret) > > + dev_alert(wdt->da9063->dev, "Watchdog failed to stop (err = %d)\n", > > + ret); > > You have a number of places where the continuation line _on purpose_ > does not align to te opening '('. I would understand it if your indentation > rules were in any way consistent, but I don't see that either. > Ok, I can see that you dislike the rule that continuation lines should be > aligned to '(', but to misalign even when the '(' matches a tab position doesn't > really make any sense. My indentation rules are consistent, otherwise I made a mistake. I always indent two tabs after an opening bracket when breaking long lines. Also I can't find any specific rules about how to break long lines in the coding style document. > > > + mutex_unlock(&wdt->lock); > > + > > + return ret; > > +} > > + > > +static int da9063_wdt_ping(struct watchdog_device *wdd) > > +{ > > + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); > > + int ret; > > + > > + mutex_lock(&wdt->lock); > > + ret = _da9063_wdt_kick(wdt->da9063); > > + if (ret) > > + dev_alert(wdt->da9063->dev, "Failed to ping the watchdog (err = %d)\n", > > + ret); > > + mutex_unlock(&wdt->lock); > > + > > + return ret; > > +} > > + > > +static int da9063_wdt_set_timeout(struct watchdog_device *wdd, > > + unsigned int timeout) > > +{ > > + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); > > + unsigned int selector; > > + int ret; > > + > > + selector = da9063_wdt_timeout_to_sel(timeout); > > + if (selector < 0) { > > selector must be an int for this to work. > > > + dev_err(wdt->da9063->dev, "Unsupported watchdog timeout, should be between 2 and 131\n"); > > Please use a continuation line here. Ok. Thank you, Markus
On Thu, Aug 21, 2014 at 08:15:25AM +0200, Markus Pargmann wrote: > Hi, > ... > > > +static int da9063_wdt_timeout_to_sel(int secs) > > > +{ > > > + int i; > > > + > > > + for (i = DA9063_TWDSCALE_MIN; i <= DA9063_TWDSCALE_MAX; i++) { > > > > When building with W=1, gcc complains about this line. Would be great if you can > > have a look and fix it. > > I just compiled with W=1, it doesn't complain here about this line. > Could you give me the warning? > drivers/watchdog/da9063_wdt.c: In function 'da9063_wdt_timeout_to_sel': drivers/watchdog/da9063_wdt.c:48:34: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] This is with arm-poky-linux-gnueabi-gcc (GCC) 4.7.2. > > > +static int da9063_wdt_stop(struct watchdog_device *wdd) > > > +{ > > > + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); > > > + int ret; > > > + > > > + mutex_lock(&wdt->lock); > > > + ret = da9063_wdt_disable(wdt->da9063); > > > + if (ret) > > > + dev_alert(wdt->da9063->dev, "Watchdog failed to stop (err = %d)\n", > > > + ret); > > > > You have a number of places where the continuation line _on purpose_ > > does not align to te opening '('. I would understand it if your indentation > > rules were in any way consistent, but I don't see that either. > > Ok, I can see that you dislike the rule that continuation lines should be > > aligned to '(', but to misalign even when the '(' matches a tab position doesn't > > really make any sense. > > My indentation rules are consistent, otherwise I made a mistake. I > always indent two tabs after an opening bracket when breaking long > lines. Also I can't find any specific rules about how to break long > lines in the coding style document. > Hmm, I thought this was an official CodingStyle rule. Guess it is just a checkpatch rule. Oh well. Guenter
On 16 August 2014 13:36, Markus Pargmann [mailto:mpa@pengutronix.de] wrote: > Subject: [PATCH v4] watchdog: Add DA906x PMIC watchdog driver. Hi Markus, > > From: Krystian Garbaciak <krystian.garbaciak@diasemi.com> > > This driver supports the watchdog device inside the DA906x PMIC. > There is only DA9063 , in this case there is no "x". Thanks. [...] > + > +/* > + * Watchdog selector to timeout in seconds. > + * 0: WDT disabled; > + * others: timeout = 2048 ms * 2^(TWDSCALE-1). > + */ > +static const int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 }; This table is just an approximation of the real times provided by the PMIC. The first three bits in the CONTROL_D register at 0x11 define a scaling value for the nominal maximum time TWDMAX. The 7 values specifying the timeout of the watchdog are: 001 = 1 = 1x = 2048ms 010 = 2 = 2x = 4096ms 011 = 3 = 4x = 8192ms 100 = 4 = 8x = 16384ms 101 = 5 = 16x = 32768ms 110 = 6 = 32x = 65536ms 111 = 7 = 64x = 131072ms [...] > + > +static int da9063_wdt_disable(struct da9063 *da9063) > +{ > + return regmap_update_bits(da9063->regmap, > DA9063_REG_CONTROL_D, > + DA9063_TWDSCALE_MASK, > + DA9063_TWDSCALE_DISABLE); > +} > + > +static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int > regval) > +{ > + return regmap_update_bits(da9063->regmap, > DA9063_REG_CONTROL_D, > + DA9063_TWDSCALE_MASK, regval); > +} > + > +static int _da9063_wdt_kick(struct da9063 *da9063) > +{ > + return regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, > + DA9063_WATCHDOG); > +} > + The _da9063_wdt_kick() and da9063_wdt_disable() functions are only used once throughout the code -- in the watchdog_ops functions da9063_wdt_ping() and da9063_wdt_stop() respectively. Since these are just simple regmap calls, could they be added at those places directly and therefore remove the function calls? [...] > +static int da9063_wdt_start(struct watchdog_device *wdd) > +{ > + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); > + unsigned int selector; > + int ret; > + > + selector = da9063_wdt_timeout_to_sel(wdt->wdtdev.timeout); > + if (selector < 0) { Unsigned if < 0 [...] > +static int da9063_wdt_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); > + unsigned int selector; > + int ret; > + > + selector = da9063_wdt_timeout_to_sel(timeout); > + if (selector < 0) { And again Regards, Steve
Hi, On Thu, Aug 21, 2014 at 07:19:59AM -0700, Guenter Roeck wrote: > On Thu, Aug 21, 2014 at 08:15:25AM +0200, Markus Pargmann wrote: > > Hi, > > > ... > > > > +static int da9063_wdt_timeout_to_sel(int secs) > > > > +{ > > > > + int i; > > > > + > > > > + for (i = DA9063_TWDSCALE_MIN; i <= DA9063_TWDSCALE_MAX; i++) { > > > > > > When building with W=1, gcc complains about this line. Would be great if you can > > > have a look and fix it. > > > > I just compiled with W=1, it doesn't complain here about this line. > > Could you give me the warning? > > > drivers/watchdog/da9063_wdt.c: In function 'da9063_wdt_timeout_to_sel': > drivers/watchdog/da9063_wdt.c:48:34: warning: comparison between signed and > unsigned integer expressions [-Wsign-compare] > > This is with arm-poky-linux-gnueabi-gcc (GCC) 4.7.2. Thanks, I am using gcc 4.8.3. I replaced the used 'int' type by 'unsigend int', that should fix it. Best regards, Markus
Hi Steve, On Fri, Aug 22, 2014 at 03:47:48PM +0000, Opensource [Steve Twiss] wrote: > On 16 August 2014 13:36, Markus Pargmann [mailto:mpa@pengutronix.de] wrote: > > > Subject: [PATCH v4] watchdog: Add DA906x PMIC watchdog driver. > > Hi Markus, > > > > > From: Krystian Garbaciak <krystian.garbaciak@diasemi.com> > > > > This driver supports the watchdog device inside the DA906x PMIC. > > > > There is only DA9063 , in this case there is no "x". Thanks. Thanks, fixed in the commit message. > > [...] > > > + > > +/* > > + * Watchdog selector to timeout in seconds. > > + * 0: WDT disabled; > > + * others: timeout = 2048 ms * 2^(TWDSCALE-1). > > + */ > > +static const int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 }; > > This table is just an approximation of the real times provided by the PMIC. > > The first three bits in the CONTROL_D register at 0x11 define a scaling value > for the nominal maximum time TWDMAX. The 7 values specifying the timeout > of the watchdog are: > > 001 = 1 = 1x = 2048ms > 010 = 2 = 2x = 4096ms > 011 = 3 = 4x = 8192ms > 100 = 4 = 8x = 16384ms > 101 = 5 = 16x = 32768ms > 110 = 6 = 32x = 65536ms > 111 = 7 = 64x = 131072ms Thanks. So the 7 values are good approximations for the real values right? > > [...] > > > + > > +static int da9063_wdt_disable(struct da9063 *da9063) > > +{ > > + return regmap_update_bits(da9063->regmap, > > DA9063_REG_CONTROL_D, > > + DA9063_TWDSCALE_MASK, > > + DA9063_TWDSCALE_DISABLE); > > +} > > + > > +static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int > > regval) > > +{ > > + return regmap_update_bits(da9063->regmap, > > DA9063_REG_CONTROL_D, > > + DA9063_TWDSCALE_MASK, regval); > > +} > > + > > +static int _da9063_wdt_kick(struct da9063 *da9063) > > +{ > > + return regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, > > + DA9063_WATCHDOG); > > +} > > + > > The _da9063_wdt_kick() and da9063_wdt_disable() functions are only used once > throughout the code -- in the watchdog_ops functions da9063_wdt_ping() and > da9063_wdt_stop() respectively. > Since these are just simple regmap calls, could they be added at those places > directly and therefore remove the function calls? Thanks, I removed those functions and included the regmap function calls directly as they are already well described by the functions from which they are called. > > [...] > > > +static int da9063_wdt_start(struct watchdog_device *wdd) > > +{ > > + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); > > + unsigned int selector; > > + int ret; > > + > > + selector = da9063_wdt_timeout_to_sel(wdt->wdtdev.timeout); > > + if (selector < 0) { > > Unsigned if < 0 That was already fixed for the next version. Thanks, Markus
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index c845527b503a..8d5c9b33552a 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -87,6 +87,13 @@ config DA9055_WATCHDOG This driver can also be built as a module. If so, the module will be called da9055_wdt. +config DA9063_WATCHDOG + tristate "Dialog DA9063 Watchdog" + depends on MFD_DA9063 + select WATCHDOG_CORE + help + Support for the watchdog in the DA9063 PMIC. + config GPIO_WATCHDOG tristate "Watchdog device controlled through GPIO-line" depends on OF_GPIO diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 7b8a91ed20e7..ce4a7632d863 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -172,6 +172,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o # Architecture Independent obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o +obj-$(CONFIG_DA9063_WATCHDOG) += da9063_wdt.o obj-$(CONFIG_GPIO_WATCHDOG) += gpio_wdt.o obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o obj-$(CONFIG_WM8350_WATCHDOG) += wm8350_wdt.o diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c new file mode 100644 index 000000000000..a2d46bc55805 --- /dev/null +++ b/drivers/watchdog/da9063_wdt.c @@ -0,0 +1,231 @@ +/* + * Watchdog driver for DA9063 PMICs. + * + * Copyright(c) 2012 Dialog Semiconductor Ltd. + * + * Author: Mariusz Wojtasik <mariusz.wojtasik@diasemi.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/watchdog.h> +#include <linux/platform_device.h> +#include <linux/uaccess.h> +#include <linux/slab.h> +#include <linux/delay.h> +#include <linux/mfd/da9063/registers.h> +#include <linux/mfd/da9063/core.h> +#include <linux/regmap.h> + +/* + * Watchdog selector to timeout in seconds. + * 0: WDT disabled; + * others: timeout = 2048 ms * 2^(TWDSCALE-1). + */ +static const int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 }; +#define DA9063_TWDSCALE_DISABLE 0 +#define DA9063_TWDSCALE_MIN 1 +#define DA9063_TWDSCALE_MAX (ARRAY_SIZE(wdt_timeout) - 1) +#define DA9063_WDT_MIN_TIMEOUT wdt_timeout[DA9063_TWDSCALE_MIN] +#define DA9063_WDT_MAX_TIMEOUT wdt_timeout[DA9063_TWDSCALE_MAX] +#define DA9063_WDG_TIMEOUT wdt_timeout[3] + +struct da9063_watchdog { + struct da9063 *da9063; + struct watchdog_device wdtdev; + struct mutex lock; +}; + +static int da9063_wdt_timeout_to_sel(int secs) +{ + int i; + + for (i = DA9063_TWDSCALE_MIN; i <= DA9063_TWDSCALE_MAX; i++) { + if (wdt_timeout[i] >= secs) + return i; + } + + return -EINVAL; +} + +static int da9063_wdt_disable(struct da9063 *da9063) +{ + return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D, + DA9063_TWDSCALE_MASK, + DA9063_TWDSCALE_DISABLE); +} + +static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval) +{ + return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D, + DA9063_TWDSCALE_MASK, regval); +} + +static int _da9063_wdt_kick(struct da9063 *da9063) +{ + return regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, + DA9063_WATCHDOG); +} + +static int da9063_wdt_start(struct watchdog_device *wdd) +{ + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); + unsigned int selector; + int ret; + + selector = da9063_wdt_timeout_to_sel(wdt->wdtdev.timeout); + if (selector < 0) { + dev_err(wdt->da9063->dev, "Timeout out of range (timeout=%d)\n", + wdt->wdtdev.timeout); + return selector; + } + + mutex_lock(&wdt->lock); + ret = _da9063_wdt_set_timeout(wdt->da9063, selector); + if (ret) { + dev_err(wdt->da9063->dev, "Watchdog failed to start (err = %d)\n", + ret); + } + mutex_unlock(&wdt->lock); + + return ret; +} + +static int da9063_wdt_stop(struct watchdog_device *wdd) +{ + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); + int ret; + + mutex_lock(&wdt->lock); + ret = da9063_wdt_disable(wdt->da9063); + if (ret) + dev_alert(wdt->da9063->dev, "Watchdog failed to stop (err = %d)\n", + ret); + mutex_unlock(&wdt->lock); + + return ret; +} + +static int da9063_wdt_ping(struct watchdog_device *wdd) +{ + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); + int ret; + + mutex_lock(&wdt->lock); + ret = _da9063_wdt_kick(wdt->da9063); + if (ret) + dev_alert(wdt->da9063->dev, "Failed to ping the watchdog (err = %d)\n", + ret); + mutex_unlock(&wdt->lock); + + return ret; +} + +static int da9063_wdt_set_timeout(struct watchdog_device *wdd, + unsigned int timeout) +{ + struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd); + unsigned int selector; + int ret; + + selector = da9063_wdt_timeout_to_sel(timeout); + if (selector < 0) { + dev_err(wdt->da9063->dev, "Unsupported watchdog timeout, should be between 2 and 131\n"); + return -EINVAL; + } + + mutex_lock(&wdt->lock); + + ret = _da9063_wdt_set_timeout(wdt->da9063, selector); + if (ret) + dev_err(wdt->da9063->dev, "Failed to set watchdog timeout (err = %d)\n", + ret); + else + wdd->timeout = wdt_timeout[selector]; + + mutex_unlock(&wdt->lock); + + return ret; +} + +static const struct watchdog_info da9063_watchdog_info = { + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, + .identity = "DA9063 Watchdog", +}; + +static const struct watchdog_ops da9063_watchdog_ops = { + .owner = THIS_MODULE, + .start = da9063_wdt_start, + .stop = da9063_wdt_stop, + .ping = da9063_wdt_ping, + .set_timeout = da9063_wdt_set_timeout, +}; + +static int da9063_wdt_probe(struct platform_device *pdev) +{ + int ret; + struct da9063 *da9063; + struct da9063_watchdog *wdt; + + if (!pdev->dev.parent) + return -EINVAL; + + da9063 = dev_get_drvdata(pdev->dev.parent); + if (!da9063) + return -EINVAL; + + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); + if (!wdt) + return -ENOMEM; + + wdt->da9063 = da9063; + mutex_init(&wdt->lock); + + wdt->wdtdev.info = &da9063_watchdog_info; + wdt->wdtdev.ops = &da9063_watchdog_ops; + wdt->wdtdev.min_timeout = DA9063_WDT_MIN_TIMEOUT; + wdt->wdtdev.max_timeout = DA9063_WDT_MAX_TIMEOUT; + wdt->wdtdev.timeout = DA9063_WDG_TIMEOUT; + + wdt->wdtdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS; + + watchdog_set_drvdata(&wdt->wdtdev, wdt); + dev_set_drvdata(&pdev->dev, wdt); + + ret = watchdog_register_device(&wdt->wdtdev); + if (ret) { + dev_err(da9063->dev, "da9063-watchdog registration failed (err = %d)", + ret); + return ret; + } + + return 0; +} + +static int da9063_wdt_remove(struct platform_device *pdev) +{ + struct da9063_watchdog *wdt = dev_get_drvdata(&pdev->dev); + + watchdog_unregister_device(&wdt->wdtdev); + + return 0; +} + +static struct platform_driver da9063_wdt_driver = { + .probe = da9063_wdt_probe, + .remove = da9063_wdt_remove, + .driver = { + .name = DA9063_DRVNAME_WATCHDOG, + }, +}; +module_platform_driver(da9063_wdt_driver); + +MODULE_AUTHOR("Mariusz Wojtasik <mariusz.wojtasik@diasemi.com>"); +MODULE_DESCRIPTION("Watchdog driver for Dialog DA9063"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:" DA9063_DRVNAME_WATCHDOG);