This patch removes the static watchdog device for a new max63xx_wdt data
structure, and constifies the max63xx_timeout data.
The new structure contains pointers to pin access routines, which
abstracts mmap-specific code. This will ease future accesses like GPIO.
Signed-off-by: Vivien Didelot <[email protected]>
---
drivers/watchdog/max63xx_wdt.c | 170 +++++++++++++++++++++++++----------------
1 file changed, 104 insertions(+), 66 deletions(-)
diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c
index b3a1130..3f7e8d5 100644
--- a/drivers/watchdog/max63xx_wdt.c
+++ b/drivers/watchdog/max63xx_wdt.c
@@ -39,10 +39,22 @@ static bool nowayout = WATCHDOG_NOWAYOUT;
#define MAX6369_WDSET (7 << 0)
#define MAX6369_WDI (1 << 3)
-static DEFINE_SPINLOCK(io_lock);
+#define MAX6369_WDSET_DISABLED 3
static int nodelay;
-static void __iomem *wdt_base;
+
+struct max63xx_wdt {
+ struct watchdog_device wdd;
+ const struct max63xx_timeout *timeout;
+
+ /* memory mapping */
+ void __iomem *base;
+ spinlock_t lock;
+
+ /* WDI and WSET bits write access routines */
+ void (*ping)(struct max63xx_wdt *wdt);
+ void (*set)(struct max63xx_wdt *wdt, u8 set);
+};
/*
* The timeout values used are actually the absolute minimum the chip
@@ -59,25 +71,25 @@ static void __iomem *wdt_base;
/* Timeouts in second */
struct max63xx_timeout {
- u8 wdset;
- u8 tdelay;
- u8 twd;
+ const u8 wdset;
+ const u8 tdelay;
+ const u8 twd;
};
-static struct max63xx_timeout max6369_table[] = {
+static const struct max63xx_timeout max6369_table[] = {
{ 5, 1, 1 },
{ 6, 10, 10 },
{ 7, 60, 60 },
{ },
};
-static struct max63xx_timeout max6371_table[] = {
+static const struct max63xx_timeout max6371_table[] = {
{ 6, 60, 3 },
{ 7, 60, 60 },
{ },
};
-static struct max63xx_timeout max6373_table[] = {
+static const struct max63xx_timeout max6373_table[] = {
{ 2, 60, 1 },
{ 5, 0, 1 },
{ 1, 3, 3 },
@@ -86,8 +98,6 @@ static struct max63xx_timeout max6373_table[] = {
{ },
};
-static struct max63xx_timeout *current_timeout;
-
static struct max63xx_timeout *
max63xx_select_timeout(struct max63xx_timeout *table, int value)
{
@@ -108,59 +118,32 @@ max63xx_select_timeout(struct max63xx_timeout *table, int value)
static int max63xx_wdt_ping(struct watchdog_device *wdd)
{
- u8 val;
-
- spin_lock(&io_lock);
+ struct max63xx_wdt *wdt = watchdog_get_drvdata(wdd);
- val = __raw_readb(wdt_base);
-
- __raw_writeb(val | MAX6369_WDI, wdt_base);
- __raw_writeb(val & ~MAX6369_WDI, wdt_base);
-
- spin_unlock(&io_lock);
+ wdt->ping(wdt);
return 0;
}
static int max63xx_wdt_start(struct watchdog_device *wdd)
{
- struct max63xx_timeout *entry = watchdog_get_drvdata(wdd);
- u8 val;
+ struct max63xx_wdt *wdt = watchdog_get_drvdata(wdd);
- spin_lock(&io_lock);
-
- val = __raw_readb(wdt_base);
- val &= ~MAX6369_WDSET;
- val |= entry->wdset;
- __raw_writeb(val, wdt_base);
-
- spin_unlock(&io_lock);
+ wdt->set(wdt, wdt->timeout->wdset);
/* check for a edge triggered startup */
- if (entry->tdelay == 0)
- max63xx_wdt_ping(wdd);
+ if (wdt->timeout->tdelay == 0)
+ wdt->ping(wdt);
return 0;
}
static int max63xx_wdt_stop(struct watchdog_device *wdd)
{
- u8 val;
+ struct max63xx_wdt *wdt = watchdog_get_drvdata(wdd);
- spin_lock(&io_lock);
-
- val = __raw_readb(wdt_base);
- val &= ~MAX6369_WDSET;
- val |= 3;
- __raw_writeb(val, wdt_base);
-
- spin_unlock(&io_lock);
+ wdt->set(wdt, MAX6369_WDSET_DISABLED);
return 0;
}
-static const struct watchdog_info max63xx_wdt_info = {
- .options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
- .identity = "max63xx Watchdog",
-};
-
static const struct watchdog_ops max63xx_wdt_ops = {
.owner = THIS_MODULE,
.start = max63xx_wdt_start,
@@ -168,49 +151,104 @@ static const struct watchdog_ops max63xx_wdt_ops = {
.ping = max63xx_wdt_ping,
};
-static struct watchdog_device max63xx_wdt_dev = {
- .info = &max63xx_wdt_info,
- .ops = &max63xx_wdt_ops,
+static const struct watchdog_info max63xx_wdt_info = {
+ .options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+ .identity = "max63xx Watchdog",
};
+static void max63xx_mmap_ping(struct max63xx_wdt *wdt)
+{
+ u8 val;
+
+ spin_lock(&wdt->lock);
+
+ val = __raw_readb(wdt->base);
+
+ __raw_writeb(val | MAX6369_WDI, wdt->base);
+ __raw_writeb(val & ~MAX6369_WDI, wdt->base);
+
+ spin_unlock(&wdt->lock);
+}
+
+static void max63xx_mmap_set(struct max63xx_wdt *wdt, u8 set)
+{
+ u8 val;
+
+ spin_lock(&wdt->lock);
+
+ val = __raw_readb(wdt->base);
+ val &= ~MAX6369_WDSET;
+ val |= set & MAX6369_WDSET;
+ __raw_writeb(val, wdt->base);
+
+ spin_unlock(&wdt->lock);
+}
+
+static int max63xx_mmap_init(struct platform_device *p, struct max63xx_wdt *wdt)
+{
+ struct resource *mem = platform_get_resource(p, IORESOURCE_MEM, 0);
+
+ wdt->base = devm_ioremap_resource(&p->dev, mem);
+ if (IS_ERR(wdt->base))
+ return PTR_ERR(wdt->base);
+
+ spin_lock_init(&wdt->lock);
+
+ wdt->ping = max63xx_mmap_ping;
+ wdt->set = max63xx_mmap_set;
+ return 0;
+}
+
static int max63xx_wdt_probe(struct platform_device *pdev)
{
- struct resource *wdt_mem;
+ struct max63xx_wdt *wdt;
struct max63xx_timeout *table;
+ int err;
+
+ wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+ if (!wdt)
+ return -ENOMEM;
table = (struct max63xx_timeout *)pdev->id_entry->driver_data;
if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT)
heartbeat = DEFAULT_HEARTBEAT;
- dev_info(&pdev->dev, "requesting %ds heartbeat\n", heartbeat);
- current_timeout = max63xx_select_timeout(table, heartbeat);
-
- if (!current_timeout) {
- dev_err(&pdev->dev, "unable to satisfy heartbeat request\n");
+ wdt->timeout = max63xx_select_timeout(table, heartbeat);
+ if (!wdt->timeout) {
+ dev_err(&pdev->dev, "unable to satisfy %ds heartbeat request\n",
+ heartbeat);
return -EINVAL;
}
- dev_info(&pdev->dev, "using %ds heartbeat with %ds initial delay\n",
- current_timeout->twd, current_timeout->tdelay);
+ err = max63xx_mmap_init(pdev, wdt);
+ if (err)
+ return err;
+
+ platform_set_drvdata(pdev, &wdt->wdd);
+ watchdog_set_drvdata(&wdt->wdd, wdt);
- heartbeat = current_timeout->twd;
+ wdt->wdd.parent = &pdev->dev;
+ wdt->wdd.timeout = wdt->timeout->twd;
+ wdt->wdd.info = &max63xx_wdt_info;
+ wdt->wdd.ops = &max63xx_wdt_ops;
- wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- wdt_base = devm_ioremap_resource(&pdev->dev, wdt_mem);
- if (IS_ERR(wdt_base))
- return PTR_ERR(wdt_base);
+ watchdog_set_nowayout(&wdt->wdd, nowayout);
- max63xx_wdt_dev.timeout = heartbeat;
- watchdog_set_nowayout(&max63xx_wdt_dev, nowayout);
- watchdog_set_drvdata(&max63xx_wdt_dev, current_timeout);
+ err = watchdog_register_device(&wdt->wdd);
+ if (err)
+ return err;
- return watchdog_register_device(&max63xx_wdt_dev);
+ dev_info(&pdev->dev, "using %ds heartbeat with %ds initial delay\n",
+ wdt->timeout->twd, wdt->timeout->tdelay);
+ return 0;
}
static int max63xx_wdt_remove(struct platform_device *pdev)
{
- watchdog_unregister_device(&max63xx_wdt_dev);
+ struct watchdog_device *wdd = platform_get_drvdata(pdev);
+
+ watchdog_unregister_device(wdd);
return 0;
}
--
2.4.3
Introduce a new struct max63xx_platform_data to support MAX63xx watchdog
chips connected via GPIO. A platform code can fill this structure with
GPIO numbers for WDI and WDSET pins to enable GPIO support in the code.
The driver takes care of requesting and releasing the GPIOs.
Signed-off-by: Vivien Didelot <[email protected]>
---
drivers/watchdog/Kconfig | 2 +-
drivers/watchdog/max63xx_wdt.c | 53 ++++++++++++++++++++++++++++++-
include/linux/platform_data/max63xx_wdt.h | 27 ++++++++++++++++
3 files changed, 80 insertions(+), 2 deletions(-)
create mode 100644 include/linux/platform_data/max63xx_wdt.h
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 742fbbc..bb65b20 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -418,7 +418,7 @@ config TS72XX_WATCHDOG
config MAX63XX_WATCHDOG
tristate "Max63xx watchdog"
- depends on HAS_IOMEM
+ depends on HAS_IOMEM || GPIOLIB
select WATCHDOG_CORE
help
Support for memory mapped max63{69,70,71,72,73,74} watchdog timer.
diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c
index 3f7e8d5..cce2b12 100644
--- a/drivers/watchdog/max63xx_wdt.c
+++ b/drivers/watchdog/max63xx_wdt.c
@@ -22,6 +22,8 @@
#include <linux/watchdog.h>
#include <linux/bitops.h>
#include <linux/platform_device.h>
+#include <linux/platform_data/max63xx_wdt.h>
+#include <linux/gpio.h>
#include <linux/spinlock.h>
#include <linux/io.h>
#include <linux/slab.h>
@@ -47,6 +49,9 @@ struct max63xx_wdt {
struct watchdog_device wdd;
const struct max63xx_timeout *timeout;
+ /* platform settings e.g. GPIO */
+ struct max63xx_platform_data *pdata;
+
/* memory mapping */
void __iomem *base;
spinlock_t lock;
@@ -199,6 +204,48 @@ static int max63xx_mmap_init(struct platform_device *p, struct max63xx_wdt *wdt)
return 0;
}
+static void max63xx_gpio_ping(struct max63xx_wdt *wdt)
+{
+ gpio_set_value_cansleep(wdt->pdata->wdi, 1);
+ gpio_set_value_cansleep(wdt->pdata->wdi, 0);
+}
+
+static void max63xx_gpio_set(struct max63xx_wdt *wdt, u8 set)
+{
+ gpio_set_value_cansleep(wdt->pdata->set0, set & BIT(0));
+ gpio_set_value_cansleep(wdt->pdata->set1, set & BIT(1));
+ gpio_set_value_cansleep(wdt->pdata->set2, set & BIT(2));
+}
+
+static int max63xx_gpio_init(struct platform_device *p, struct max63xx_wdt *wdt)
+{
+ int err;
+
+ err = devm_gpio_request_one(&p->dev, wdt->pdata->wdi,
+ GPIOF_OUT_INIT_LOW, "max63xx_wdt WDI");
+ if (err)
+ return err;
+
+ err = devm_gpio_request_one(&p->dev, wdt->pdata->set0,
+ GPIOF_OUT_INIT_LOW, "max63xx_wdt SET0");
+ if (err)
+ return err;
+
+ err = devm_gpio_request_one(&p->dev, wdt->pdata->set1,
+ GPIOF_OUT_INIT_LOW, "max63xx_wdt SET1");
+ if (err)
+ return err;
+
+ err = devm_gpio_request_one(&p->dev, wdt->pdata->set2,
+ GPIOF_OUT_INIT_LOW, "max63xx_wdt SET2");
+ if (err)
+ return err;
+
+ wdt->ping = max63xx_gpio_ping;
+ wdt->set = max63xx_gpio_set;
+ return 0;
+}
+
static int max63xx_wdt_probe(struct platform_device *pdev)
{
struct max63xx_wdt *wdt;
@@ -211,6 +258,8 @@ static int max63xx_wdt_probe(struct platform_device *pdev)
table = (struct max63xx_timeout *)pdev->id_entry->driver_data;
+ wdt->pdata = dev_get_platdata(&pdev->dev);
+
if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT)
heartbeat = DEFAULT_HEARTBEAT;
@@ -221,7 +270,9 @@ static int max63xx_wdt_probe(struct platform_device *pdev)
return -EINVAL;
}
- err = max63xx_mmap_init(pdev, wdt);
+ /* GPIO or memory mapped? */
+ err = wdt->pdata && wdt->pdata->wdi ? max63xx_gpio_init(pdev, wdt) :
+ max63xx_mmap_init(pdev, wdt);
if (err)
return err;
diff --git a/include/linux/platform_data/max63xx_wdt.h b/include/linux/platform_data/max63xx_wdt.h
new file mode 100644
index 0000000..ae28024
--- /dev/null
+++ b/include/linux/platform_data/max63xx_wdt.h
@@ -0,0 +1,27 @@
+/*
+ * Maxim MAX6369 Pin-Selectable Watchdog Timer and compatibles
+ *
+ * Copyright (c) 2015 Savoir-faire Linux Inc.
+ * Vivien Didelot <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _PDATA_MAX63XX_H
+#define _PDATA_MAX63XX_H
+
+/**
+ * struct max63xx_platform_data - MAX63xx connectivity info
+ * @wdi: Watchdog Input GPIO number.
+ * @set0: Watchdog SET0 GPIO number.
+ * @set1: Watchdog SET1 GPIO number.
+ * @set2: Watchdog SET2 GPIO number.
+ */
+struct max63xx_platform_data {
+ unsigned int wdi;
+ unsigned int set0, set1, set2;
+};
+
+#endif /* _PDATA_MAX63XX_H */
--
2.4.3
Actually, there is no way but the module parameter to set the desired
heartbeat. This patch allows a platform code to set it in the device
platform data. This is convenient for platforms and built-in drivers.
To do so, initialize heartbeat to zero to allow the module parameter to
take precedence over the platform setting. If not set, it will still
default to DEFAULT_HEARTBEAT.
Signed-off-by: Vivien Didelot <[email protected]>
---
drivers/watchdog/max63xx_wdt.c | 6 +++++-
include/linux/platform_data/max63xx_wdt.h | 2 ++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c
index cce2b12..e49ecd9 100644
--- a/drivers/watchdog/max63xx_wdt.c
+++ b/drivers/watchdog/max63xx_wdt.c
@@ -31,7 +31,7 @@
#define DEFAULT_HEARTBEAT 60
#define MAX_HEARTBEAT 60
-static unsigned int heartbeat = DEFAULT_HEARTBEAT;
+static unsigned int heartbeat;
static bool nowayout = WATCHDOG_NOWAYOUT;
/*
@@ -260,6 +260,10 @@ static int max63xx_wdt_probe(struct platform_device *pdev)
wdt->pdata = dev_get_platdata(&pdev->dev);
+ /* The module parameter takes precedence over the platform data */
+ if (!heartbeat && wdt->pdata && wdt->pdata->heartbeat)
+ heartbeat = wdt->pdata->heartbeat;
+
if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT)
heartbeat = DEFAULT_HEARTBEAT;
diff --git a/include/linux/platform_data/max63xx_wdt.h b/include/linux/platform_data/max63xx_wdt.h
index ae28024..8127b1a 100644
--- a/include/linux/platform_data/max63xx_wdt.h
+++ b/include/linux/platform_data/max63xx_wdt.h
@@ -14,12 +14,14 @@
/**
* struct max63xx_platform_data - MAX63xx connectivity info
+ * @heartbeat: Watchdog heartbeat period in seconds.
* @wdi: Watchdog Input GPIO number.
* @set0: Watchdog SET0 GPIO number.
* @set1: Watchdog SET1 GPIO number.
* @set2: Watchdog SET2 GPIO number.
*/
struct max63xx_platform_data {
+ unsigned int heartbeat;
unsigned int wdi;
unsigned int set0, set1, set2;
};
--
2.4.3
Hi Vivien,
On Wed, Jun 17, 2015 at 06:58:58PM -0400, Vivien Didelot wrote:
> This patch removes the static watchdog device for a new max63xx_wdt data
> structure, and constifies the max63xx_timeout data.
>
> The new structure contains pointers to pin access routines, which
> abstracts mmap-specific code. This will ease future accesses like GPIO.
>
> Signed-off-by: Vivien Didelot <[email protected]>
Looks good. Minor nitpick below, but not worth a new version.
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> drivers/watchdog/max63xx_wdt.c | 170 +++++++++++++++++++++++++----------------
> 1 file changed, 104 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c
> index b3a1130..3f7e8d5 100644
> --- a/drivers/watchdog/max63xx_wdt.c
> +++ b/drivers/watchdog/max63xx_wdt.c
> @@ -39,10 +39,22 @@ static bool nowayout = WATCHDOG_NOWAYOUT;
> #define MAX6369_WDSET (7 << 0)
> #define MAX6369_WDI (1 << 3)
>
> -static DEFINE_SPINLOCK(io_lock);
> +#define MAX6369_WDSET_DISABLED 3
I would have used a tab before '3'.
Thanks,
Guenter
Hi Vivien,
On Wed, Jun 17, 2015 at 06:58:59PM -0400, Vivien Didelot wrote:
> Introduce a new struct max63xx_platform_data to support MAX63xx watchdog
> chips connected via GPIO. A platform code can fill this structure with
> GPIO numbers for WDI and WDSET pins to enable GPIO support in the code.
>
> The driver takes care of requesting and releasing the GPIOs.
>
> Signed-off-by: Vivien Didelot <[email protected]>
would it be possible to use gpiod functions ?
> ---
> drivers/watchdog/Kconfig | 2 +-
> drivers/watchdog/max63xx_wdt.c | 53 ++++++++++++++++++++++++++++++-
> include/linux/platform_data/max63xx_wdt.h | 27 ++++++++++++++++
> 3 files changed, 80 insertions(+), 2 deletions(-)
> create mode 100644 include/linux/platform_data/max63xx_wdt.h
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 742fbbc..bb65b20 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -418,7 +418,7 @@ config TS72XX_WATCHDOG
>
> config MAX63XX_WATCHDOG
> tristate "Max63xx watchdog"
> - depends on HAS_IOMEM
> + depends on HAS_IOMEM || GPIOLIB
> select WATCHDOG_CORE
> help
> Support for memory mapped max63{69,70,71,72,73,74} watchdog timer.
> diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c
> index 3f7e8d5..cce2b12 100644
> --- a/drivers/watchdog/max63xx_wdt.c
> +++ b/drivers/watchdog/max63xx_wdt.c
> @@ -22,6 +22,8 @@
> #include <linux/watchdog.h>
> #include <linux/bitops.h>
> #include <linux/platform_device.h>
> +#include <linux/platform_data/max63xx_wdt.h>
> +#include <linux/gpio.h>
> #include <linux/spinlock.h>
> #include <linux/io.h>
> #include <linux/slab.h>
> @@ -47,6 +49,9 @@ struct max63xx_wdt {
> struct watchdog_device wdd;
> const struct max63xx_timeout *timeout;
>
> + /* platform settings e.g. GPIO */
> + struct max63xx_platform_data *pdata;
> +
> /* memory mapping */
> void __iomem *base;
> spinlock_t lock;
> @@ -199,6 +204,48 @@ static int max63xx_mmap_init(struct platform_device *p, struct max63xx_wdt *wdt)
> return 0;
> }
>
> +static void max63xx_gpio_ping(struct max63xx_wdt *wdt)
> +{
> + gpio_set_value_cansleep(wdt->pdata->wdi, 1);
> + gpio_set_value_cansleep(wdt->pdata->wdi, 0);
> +}
> +
> +static void max63xx_gpio_set(struct max63xx_wdt *wdt, u8 set)
> +{
> + gpio_set_value_cansleep(wdt->pdata->set0, set & BIT(0));
> + gpio_set_value_cansleep(wdt->pdata->set1, set & BIT(1));
> + gpio_set_value_cansleep(wdt->pdata->set2, set & BIT(2));
> +}
> +
> +static int max63xx_gpio_init(struct platform_device *p, struct max63xx_wdt *wdt)
> +{
> + int err;
> +
> + err = devm_gpio_request_one(&p->dev, wdt->pdata->wdi,
> + GPIOF_OUT_INIT_LOW, "max63xx_wdt WDI");
> + if (err)
> + return err;
> +
> + err = devm_gpio_request_one(&p->dev, wdt->pdata->set0,
> + GPIOF_OUT_INIT_LOW, "max63xx_wdt SET0");
> + if (err)
> + return err;
> +
> + err = devm_gpio_request_one(&p->dev, wdt->pdata->set1,
> + GPIOF_OUT_INIT_LOW, "max63xx_wdt SET1");
> + if (err)
> + return err;
> +
> + err = devm_gpio_request_one(&p->dev, wdt->pdata->set2,
> + GPIOF_OUT_INIT_LOW, "max63xx_wdt SET2");
> + if (err)
> + return err;
> +
> + wdt->ping = max63xx_gpio_ping;
> + wdt->set = max63xx_gpio_set;
> + return 0;
> +}
> +
> static int max63xx_wdt_probe(struct platform_device *pdev)
> {
> struct max63xx_wdt *wdt;
> @@ -211,6 +258,8 @@ static int max63xx_wdt_probe(struct platform_device *pdev)
>
> table = (struct max63xx_timeout *)pdev->id_entry->driver_data;
>
> + wdt->pdata = dev_get_platdata(&pdev->dev);
> +
> if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT)
> heartbeat = DEFAULT_HEARTBEAT;
>
> @@ -221,7 +270,9 @@ static int max63xx_wdt_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> - err = max63xx_mmap_init(pdev, wdt);
> + /* GPIO or memory mapped? */
> + err = wdt->pdata && wdt->pdata->wdi ? max63xx_gpio_init(pdev, wdt) :
> + max63xx_mmap_init(pdev, wdt);
I would prefer something like
if (wdt->pdata && wdt->pdata->wdi)
err = max63xx_gpio_init(pdev, wdt);
else
err = max63xx_mmap_init(pdev, wdt);
because it is much easier to read.
Also, you might want to use gpio_is_valid(). pin number 0 is a valid pin
for at least some architectures.
Thanks,
Guenter
Hi Vivien,
On Wed, Jun 17, 2015 at 06:59:00PM -0400, Vivien Didelot wrote:
> Actually, there is no way but the module parameter to set the desired
> heartbeat. This patch allows a platform code to set it in the device
> platform data. This is convenient for platforms and built-in drivers.
>
> To do so, initialize heartbeat to zero to allow the module parameter to
> take precedence over the platform setting. If not set, it will still
> default to DEFAULT_HEARTBEAT.
>
I think that warrants a bit of discussion. Is the chip used on an
x86 system (no devicetree), and is there reason to believe that the
default watchdog timeout is not good enough until the watchdog application
starts and can configure it to a different value ?
This is also a bit more complicated since gpio pin 0 can be a valid gpio
pin number, so you'd have to explicitly state "don't use gpio" in the
platform data.
Thanks,
Guenter
Hi Guenter,
On Jun 22, 2015, at 12:53 PM, Guenter Roeck [email protected] wrote:
> On Wed, Jun 17, 2015 at 06:58:59PM -0400, Vivien Didelot wrote:
>> Introduce a new struct max63xx_platform_data to support MAX63xx watchdog
>> chips connected via GPIO. A platform code can fill this structure with
>> GPIO numbers for WDI and WDSET pins to enable GPIO support in the code.
>>
>> The driver takes care of requesting and releasing the GPIOs.
>>
>> Signed-off-by: Vivien Didelot <[email protected]>
>
> would it be possible to use gpiod functions ?
It might be, but I never played with it yet though. I'm using integer-based
GPIOs from a TCA6424 on an x86 platform (no Device Tree). Is it ok to keep
max63xx_gpio_{ping,set} for legacy GPIOs, and let someone add
max63xx_gpiod_{ping,set} if there is a need?
Thanks,
-v
Hi Guenter,
On Jun 22, 2015, at 12:59 PM, Guenter Roeck [email protected] wrote:
> Hi Vivien,
>
> On Wed, Jun 17, 2015 at 06:59:00PM -0400, Vivien Didelot wrote:
>> Actually, there is no way but the module parameter to set the desired
>> heartbeat. This patch allows a platform code to set it in the device
>> platform data. This is convenient for platforms and built-in drivers.
>>
>> To do so, initialize heartbeat to zero to allow the module parameter to
>> take precedence over the platform setting. If not set, it will still
>> default to DEFAULT_HEARTBEAT.
>
> I think that warrants a bit of discussion. Is the chip used on an
> x86 system (no devicetree), and is there reason to believe that the
> default watchdog timeout is not good enough until the watchdog application
> starts and can configure it to a different value ?
Indeed, I am using a MAX6373 device on an embedded Atom platform. The
default 60s heartbeat is not valid for this chip. I need the setting
with 10s heartbeat and 60s delay.
> This is also a bit more complicated since gpio pin 0 can be a valid gpio
> pin number, so you'd have to explicitly state "don't use gpio" in the
> platform data.
Indeed. Also, you may have a gpio pin 0 and don't want to use it. I'd
prefer to avoid any additional boolean if possible. Having at least one
positive integer seems safe. Would this be better?
/* GPIO or memory mapped? */
if (platform_get_resource(pdev, IORESOURCE_MEM, 0))
err = max63xx_mmap_init(pdev, wdt);
else if (wdt->pdata && (wdt->pdata->wdi || wdt->pdata->set0 ||
wdt->pdata->set1 || wdt->pdata->set2))
err = max63xx_gpio_init(pdev, wdt);
else
err = -EINVAL;
if (err)
return err;
Thanks,
-v
On 06/22/2015 01:43 PM, Vivien Didelot wrote:
> Hi Guenter,
>
> On Jun 22, 2015, at 12:53 PM, Guenter Roeck [email protected] wrote:
>> On Wed, Jun 17, 2015 at 06:58:59PM -0400, Vivien Didelot wrote:
>>> Introduce a new struct max63xx_platform_data to support MAX63xx watchdog
>>> chips connected via GPIO. A platform code can fill this structure with
>>> GPIO numbers for WDI and WDSET pins to enable GPIO support in the code.
>>>
>>> The driver takes care of requesting and releasing the GPIOs.
>>>
>>> Signed-off-by: Vivien Didelot <[email protected]>
>>
>> would it be possible to use gpiod functions ?
>
> It might be, but I never played with it yet though. I'm using integer-based
> GPIOs from a TCA6424 on an x86 platform (no Device Tree). Is it ok to keep
> max63xx_gpio_{ping,set} for legacy GPIOs, and let someone add
> max63xx_gpiod_{ping,set} if there is a need?
>
Hi Vivien,
That would pretty much defeat the purpose. The gpiod API is supposed
to replace the gpio API, not to augment it. Having both at the same time
does not really make sense.
There is a mapping from integer based pins to name based pins; check out
gpiod_add_lookup_table(). Essentially platform initialization code
(in your case probably the code which instantiates the tca6424) would
set up a pin lookup table, and then you would request pins using
a name instead of a number. That would also solve the "is the pin
valid" problem in patch 3/3 since you would have a string to identify
the gpio pin.
Thanks,
Guenter
Hi Guenter,
On Jun 27, 2015, at 12:17 PM, Guenter Roeck [email protected] wrote:
> On 06/22/2015 01:43 PM, Vivien Didelot wrote:
>> Hi Guenter,
>>
>> On Jun 22, 2015, at 12:53 PM, Guenter Roeck [email protected] wrote:
>>> On Wed, Jun 17, 2015 at 06:58:59PM -0400, Vivien Didelot wrote:
>>>> Introduce a new struct max63xx_platform_data to support MAX63xx watchdog
>>>> chips connected via GPIO. A platform code can fill this structure with
>>>> GPIO numbers for WDI and WDSET pins to enable GPIO support in the code.
>>>>
>>>> The driver takes care of requesting and releasing the GPIOs.
>>>>
>>>> Signed-off-by: Vivien Didelot <[email protected]>
>>>
>>> would it be possible to use gpiod functions ?
>>
>> It might be, but I never played with it yet though. I'm using integer-based
>> GPIOs from a TCA6424 on an x86 platform (no Device Tree). Is it ok to keep
>> max63xx_gpio_{ping,set} for legacy GPIOs, and let someone add
>> max63xx_gpiod_{ping,set} if there is a need?
>>
>
> Hi Vivien,
>
> That would pretty much defeat the purpose. The gpiod API is supposed
> to replace the gpio API, not to augment it. Having both at the same time
> does not really make sense.
>
> There is a mapping from integer based pins to name based pins; check out
> gpiod_add_lookup_table(). Essentially platform initialization code
> (in your case probably the code which instantiates the tca6424) would
> set up a pin lookup table, and then you would request pins using
> a name instead of a number. That would also solve the "is the pin
> valid" problem in patch 3/3 since you would have a string to identify
> the gpio pin.
Unless I'm missing something, it seems like it won't work in my case. Here's my
setup. I have 2 TCA6424 I2C I/O expanders, exposing 24 GPIOs each. They are
registered like this:
static struct i2c_board_info i2c_devices[] = {
{
I2C_BOARD_INFO("tca6424", 0x22),
.platform_data = &tca6424_1_pdata, // base = 100
}, {
I2C_BOARD_INFO("tca6424", 0x23),
.platform_data = &tca6424_2_pdata, // base = 200
}
};
Then the pca953x driver adds two gpio chips, both labeled with the device name,
i.e. "tca6424".
I have to pass pins 219 to 222 to the max63xx_platform_data. I can declare a
GPIO lookup table like this:
static struct gpiod_lookup_table lookup_table = {
.dev_id = "max6373_wdt",
.table = {
GPIOD_LOOKUP("tca6424", 19, "MAX6373 WDI", GPIO_ACTIVE_HIGH),
GPIOD_LOOKUP_IDX("tca6424", 20, "MAX6373 SET", 0, GPIO_ACTIVE_HIGH),
GPIOD_LOOKUP_IDX("tca6424", 21, "MAX6373 SET", 1, GPIO_ACTIVE_HIGH),
GPIOD_LOOKUP_IDX("tca6424", 22, "MAX6373 SET", 2, GPIO_ACTIVE_HIGH),
}
};
but from what I've seen, the gpio_chip lookup by name will always return the
first TCA6424 instance.
To me, it looks like the gpiod API doesn't support several GPIO chips with the
same name. Either this must be fixed, or I should find a way to give the 2 I/O
expanders different names (e.g. "tca6424.0" and "tca6424.1").
As struct gpiod_lookup are meant for platform code, would that be bad if they
ignore the chip_label and care about a global GPIO number (i.e. base + hwnum)?
Do you have an idea for this setup?
Thanks,
-v