watchdog driver for embedded systems with a supervisor watchdog (MAX823 or so)
connected to a gpio. This is the platform_driver and needs platform_data for
defining the gpio pin and the watchdog timeout.
Signed-off-by: Marc Pignat <[email protected]>
---
Hi!
If you've got a max823, 824 or any cpu supervisor like this connected to your
cpu by a gpio pin, you can use this watchdog driver.
Simply add this to your board specific setup file:
#include <linux/gpio_wdt.h>
static struct gpio_wdt_pdata my_wdt = {
/* WDI input connected to this gpio */
.pin = AT91_PIN_PD27,
/* The *min* timeout */
.timeout_ms = 1140
};
static struct platform_device my_watchdog_device =
{
.name = GPIO_WDT_DRIVER_NAME,
.id = 0,
.dev = {
.platform_data=&my_wdt,
}
};
then
platform_device_register(&my_watchdog_device);
This patch is against 2.6.24-rc7-git2
Comments are welcome
Regards
Marc
diff -urN a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
--- a/drivers/watchdog/gpio_wdt.c 1970-01-01 01:00:00.000000000 +0100
+++ b/drivers/watchdog/gpio_wdt.c 2008-01-10 16:06:09.000000000 +0100
@@ -0,0 +1,300 @@
+/*
+ * Generic GPIO based watchdog driver
+ *
+ * Copyright (C) 2007 Marc Pignat for http://www.hevs.ch
+ *
+ * This is a driver for the MAXIM MAX823 (and similar watchdog chips) connected
+ * on a generic gpio line.
+ *
+ * Principle of operation:
+ *
+ * When the WDI input is not driven (or in high impedance state), the
+ * watchdog is disabled. This mode of operations supposes that the gpio line
+ * can be put in high-impedance state (input) and is not driving too much
+ * current from the WDI pin.
+ *
+ * When the WDI input is driven, it must be toggled before the timeout,
+ * otherwise a reset is generated.
+ *
+ * Other supported chips:
+ * MAXIM: MAX824, ...
+ *
+ * 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.
+ */
+
+#include <linux/pm.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <linux/gpio_wdt.h>
+#include <linux/fs.h>
+#include <linux/reboot.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+
+#include <asm/uaccess.h>
+#include <asm/arch/gpio.h>
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+struct gpio_wdt {
+ struct gpio_wdt_pdata *pdata;
+ unsigned long driver_open;
+ char expect_close;
+};
+
+static struct gpio_wdt *watchdog;
+
+#define PFX GPIO_WDT_DRIVER_NAME":"
+
+static int gpio_wdt_keepalive(struct gpio_wdt *wdt)
+{
+ gpio_set_value(wdt->pdata->pin, 0);
+ gpio_set_value(wdt->pdata->pin, 1);
+ return 0;
+}
+
+static int gpio_wdt_stop(struct gpio_wdt *wdt)
+{
+ gpio_wdt_keepalive(wdt);
+ gpio_direction_input(wdt->pdata->pin);
+ return 0;
+}
+
+static int gpio_wdt_start(struct gpio_wdt *wdt)
+{
+ gpio_direction_output(wdt->pdata->pin, 0);
+ gpio_wdt_keepalive(wdt);
+ return 0;
+}
+
+static int gpio_wdt_open(struct inode *inode, struct file *file)
+{
+ if (test_and_set_bit(0, &watchdog->driver_open))
+ return -EBUSY;
+
+ gpio_wdt_start(watchdog);
+ gpio_wdt_keepalive(watchdog);
+
+ return nonseekable_open(inode, file);
+}
+
+static int gpio_wdt_release(struct inode *inode, struct file *file)
+{
+ if (watchdog->expect_close == 42) {
+ gpio_wdt_stop(watchdog);
+ module_put(THIS_MODULE);
+ } else {
+ printk(KERN_CRIT PFX "Unexpected close, not stopping watchdog!\n");
+ gpio_wdt_keepalive(watchdog);
+ }
+ clear_bit(0, &watchdog->driver_open);
+ watchdog->expect_close = 0;
+ return 0;
+}
+
+static ssize_t gpio_wdt_write(struct file *file, const char __user *data, size_t len, loff_t *ppos)
+{
+ /*
+ * Refresh the timer.
+ */
+ if (len) {
+ if (!nowayout) {
+ size_t i;
+
+ /* In case it was set long ago */
+ watchdog->expect_close = 0;
+
+ for (i = 0; i != len; i++) {
+ char c;
+
+ if (get_user(c, data + i))
+ return -EFAULT;
+ if (c == 'V')
+ watchdog->expect_close = 42;
+ }
+ }
+ gpio_wdt_keepalive(watchdog);
+ }
+ return len;
+
+ gpio_wdt_keepalive(watchdog);
+ return len;
+}
+
+static struct watchdog_info ident = {
+ .options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+ .firmware_version = 0,
+ .identity = GPIO_WDT_DRIVER_NAME,
+};
+
+static int gpio_wdt_ioctl(struct inode *inode, struct file *file,
+ unsigned int cmd, unsigned long arg)
+{
+ struct gpio_wdt *wdt = (struct gpio_wdt *)file->private_data;
+ void __user *argp = (void __user *)arg;
+ int __user *p = argp;
+ u32 timeout = wdt->pdata->timeout_ms;
+
+ switch (cmd) {
+ case WDIOC_GETSUPPORT:
+ return copy_to_user(argp, &ident, sizeof(ident)) ? -EFAULT : 0;
+ case WDIOC_GETSTATUS:
+ case WDIOC_GETBOOTSTATUS:
+ return put_user(0, p);
+ case WDIOC_KEEPALIVE:
+ gpio_wdt_keepalive(wdt);
+ return 0;
+ case WDIOC_GETTIMEOUT:
+ return put_user(timeout, p);
+ case WDIOC_SETTIMEOUT:
+ return -EINVAL;
+ default:
+ return -ENOIOCTLCMD;
+ }
+}
+
+static int gpio_wdt_notify_sys(struct notifier_block *this,
+ unsigned long code, void *unused)
+{
+ if (code == SYS_DOWN || code == SYS_HALT)
+ gpio_wdt_stop(watchdog);
+
+ return NOTIFY_DONE;
+}
+
+static struct file_operations gpio_wdt_fops = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .write = gpio_wdt_write,
+ .ioctl = gpio_wdt_ioctl,
+ .open = gpio_wdt_open,
+ .release = gpio_wdt_release,
+};
+
+static struct miscdevice gpio_wdt_miscdev = {
+ .minor = WATCHDOG_MINOR,
+ .name = "watchdog",
+ .fops = &gpio_wdt_fops,
+};
+
+static struct notifier_block gpio_wdt_notifier = {
+ .notifier_call = gpio_wdt_notify_sys,
+};
+
+static char banner[] __initdata = KERN_INFO PFX "fixed %d.%03d seconds timeout (nowayout= %d)\n";
+
+static int __init gpio_wdt_probe(struct platform_device *pdev)
+{
+ struct gpio_wdt_pdata *pdata = (struct gpio_wdt_pdata *)pdev->dev.platform_data;
+ int ret;
+
+ if (watchdog) {
+ printk(KERN_ERR PFX "only one device supported\n");
+ return -ENODEV;
+ }
+
+ if (!pdata) {
+ printk(KERN_ERR PFX "no private data\n");
+ return -ENODEV;
+ }
+
+ watchdog = kzalloc(sizeof(*watchdog), GFP_KERNEL);
+
+ if (!watchdog)
+ return -ENOMEM;
+
+ watchdog->pdata = pdata;
+
+ if (watchdog->pdata->pin == 0 || watchdog->pdata->timeout_ms == 0) {
+ kfree(watchdog);
+ printk(KERN_ERR PFX "invalid private data\n");
+ return -ENODEV;
+ }
+
+ ret = register_reboot_notifier(&gpio_wdt_notifier);
+ if (ret) {
+ kfree(watchdog);
+ printk(KERN_ERR PFX "cannot register reboot notifier (err=%d)\n", ret);
+ return ret;
+ }
+
+ ret = misc_register(&gpio_wdt_miscdev);
+ if (ret) {
+ unregister_reboot_notifier(&gpio_wdt_notifier);
+ kfree(watchdog);
+ printk(KERN_ERR PFX "cannot register miscdev on minor=%d (err=%d)\n",
+ WATCHDOG_MINOR, ret);
+ return ret;
+ }
+
+ printk(banner, watchdog->pdata->timeout_ms/1000, watchdog->pdata->timeout_ms%1000, nowayout);
+
+ return 0;
+}
+
+static int __exit gpio_wdt_remove(struct platform_device *pdev)
+{
+ misc_deregister(&gpio_wdt_miscdev);
+ unregister_reboot_notifier(&gpio_wdt_notifier);
+ kfree(watchdog);
+ watchdog = NULL;
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+int gpio_wdt_suspend(struct platform_device *pdev, pm_message_t state)
+{
+ gpio_wdt_stop(watchdog);
+ return 0;
+}
+
+int gpio_wdt_resume(struct platform_device *pdev)
+{
+ if (test_bit(0, &watchdog->driver_open))
+ gpio_wdt_start(watchdog);
+ return 0;
+}
+
+#else
+#define gpio_wdt_suspend()
+#define gpio_wdt_resume()
+#endif
+
+/* the device driver */
+static struct platform_driver gpio_wdt_driver = {
+ .probe = gpio_wdt_probe,
+ .remove = __exit_p(gpio_wdt_remove),
+ .suspend = gpio_wdt_suspend,
+ .resume = gpio_wdt_resume,
+ .driver = {
+ .name = GPIO_WDT_DRIVER_NAME,
+ .owner = THIS_MODULE,
+ },
+};
+
+/* module init or kerel entry point */
+static int __init gpio_wdt_driver_init(void)
+{
+ return platform_driver_register(&gpio_wdt_driver);
+}
+
+/* module exit */
+static void __exit gpio_wdt_driver_exit(void)
+{
+ platform_driver_unregister(&gpio_wdt_driver);
+}
+
+module_init(gpio_wdt_driver_init);
+module_exit(gpio_wdt_driver_exit);
+
+MODULE_AUTHOR("Marc Pignat");
+MODULE_DESCRIPTION("Watchdog driver on a generic gpio line");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
diff -urN a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
--- a/drivers/watchdog/Kconfig 2008-01-08 17:30:30.000000000 +0100
+++ b/drivers/watchdog/Kconfig 2008-01-10 14:00:30.000000000 +0100
@@ -55,6 +55,13 @@
To compile this driver as a module, choose M here: the
module will be called softdog.
+config GPIO_WATCHDOG
+ tristate "Support for GPIO connected watchdog"
+ depends on GENERIC_GPIO
+ help
+ This option enables support for a MAX823-like watchdog connected to
+ GPIO input/output.
+
# ALPHA Architecture
# ARM Architecture
diff -urN a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
--- a/drivers/watchdog/Makefile 2008-01-08 17:30:30.000000000 +0100
+++ b/drivers/watchdog/Makefile 2008-01-10 14:01:32.000000000 +0100
@@ -121,3 +121,4 @@
# Architecture Independant
obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o
+obj-$(CONFIG_GPIO_WATCHDOG) += gpio_wdt.o
--- a/include/linux/gpio_wdt.h 1970-01-01 01:00:00.000000000 +0100
+++ b/include/linux/gpio_wdt.h 2008-01-10 15:48:42.000000000 +0100
@@ -0,0 +1,30 @@
+/*
+ * linux/include/gpio_watchdog.h
+ *
+ * Copyright (C) 2007 Marc Pignat for http://www.hevs.ch
+ *
+ * Watchdog using a MAX823 (or similar) watchdog input connected to a
+ * generic gpio line.
+ *
+ * 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.
+ *
+ */
+
+#ifndef _LINUX_LINUX_GPIO_WDT_H
+#define _LINUX_LINUX_GPIO_WDT_H
+
+#include <asm/arch/hardware.h>
+
+#define GPIO_WDT_DRIVER_NAME "gpio_wdt"
+
+struct gpio_wdt_pdata {
+ /* the pin as defined in asm/arch/gpio.h */
+ u32 pin;
+ /* the timeout in milliseconds */
+ u32 timeout_ms;
+};
+
+#endif
On Thu, Jan 10, 2008 at 04:11:08PM +0100, Marc Pignat wrote:
> watchdog driver for embedded systems with a supervisor watchdog (MAX823 or so)
> connected to a gpio. This is the platform_driver and needs platform_data for
> defining the gpio pin and the watchdog timeout.
>
> Signed-off-by: Marc Pignat <[email protected]>
> ---
>
> Hi!
>
> If you've got a max823, 824 or any cpu supervisor like this connected to your
> cpu by a gpio pin, you can use this watchdog driver.
>
> Simply add this to your board specific setup file:
>
> #include <linux/gpio_wdt.h>
>
> static struct gpio_wdt_pdata my_wdt = {
> /* WDI input connected to this gpio */
> .pin = AT91_PIN_PD27,
> /* The *min* timeout */
> .timeout_ms = 1140
> };
>
> static struct platform_device my_watchdog_device =
> {
> .name = GPIO_WDT_DRIVER_NAME,
> .id = 0,
> .dev = {
> .platform_data=&my_wdt,
minor style problem here, ".platform_data = &my_wdt"
[snip]
--
Ben ([email protected], http://www.fluff.org/)
'a smiley only costs 4 bytes'
Hello Marc,
Your patch looks good to me, and I have been waiting for something like
this :). Other comments below.
Le jeudi 10 janvier 2008, Marc Pignat a ?crit?:
> +#include <asm/uaccess.h>
> +#include <asm/arch/gpio.h>
At first sight, this will only work with the platforms which have implemented
gpiolib, some (like MIPS) still use the old generic GPIO API. So I think the
include will not work here for anything than ARM, correct me if I am wrong.
> +static int gpio_wdt_keepalive(struct gpio_wdt *wdt)
> +{
> + gpio_set_value(wdt->pdata->pin, 0);
> + gpio_set_value(wdt->pdata->pin, 1);
> + return 0;
> +}
Some watchdog drivers requires you to toggle the full GPIO line, not only one
bit (I particularly think about the MTX-1 board watchdog).
Did you look into hooking into Wim's uniform watchdog driver :
http://git.kernel.org/?p=linux/kernel/git/wim/linux-2.6-watchdog-experimental.git;a=commit;h=732c54027e6c866f98857c4a6d1c6c466459dcd5
Maybe you can save some code ?
--
Cordialement, Florian Fainelli
------------------------------
On Friday 11 January 2008, Florian Fainelli wrote:
> Hello Marc,
>
> Your patch looks good to me, and I have been waiting for something like
> this :). Other comments below.
>
> Le jeudi 10 janvier 2008, Marc Pignat a ?crit?:
> > +#include <asm/uaccess.h>
> > +#include <asm/arch/gpio.h>
>
> At first sight, this will only work with the platforms which have implemented
> gpiolib, some (like MIPS) still use the old generic GPIO API. So I think the
> include will not work here for anything than ARM, correct me if I am wrong.
find arch -name "Kconfig" -exec grep -q "GENERIC_GPIO" {} \; -print
arch/arm/Kconfig
arch/mips/Kconfig
arch/avr32/Kconfig
arch/blackfin/Kconfig
It seems that no only arm are using GENERIC_GPIO, even some mips chips.
I probably missed wirting the subject right, should be "watchdog on GENERIC_GPIO" :)
...
>
> Did you look into hooking into Wim's uniform watchdog driver :
>
> http://git.kernel.org/?p=linux/kernel/git/wim/linux-2.6-watchdog-experimental.git;a=commit;h=732c54027e6c866f98857c4a6d1c6c466459dcd5
Now yes, interesting!
Regards
Marc
On Jan 10, 2008 10:11 AM, Marc Pignat <[email protected]> wrote:
> watchdog driver for embedded systems with a supervisor watchdog (MAX823 or so)
> connected to a gpio. This is the platform_driver and needs platform_data for
> defining the gpio pin and the watchdog timeout.
generic gpio watchdog is good stuff ...
> #include <linux/gpio_wdt.h>
perhaps "watchdog" rather than "wdt" considering it's already
"linux/watchdog.h" ?
> --- a/drivers/watchdog/gpio_wdt.c 1970-01-01 01:00:00.000000000 +0100
> +++ b/drivers/watchdog/gpio_wdt.c 2008-01-10 16:06:09.000000000 +0100
> +#include <asm/arch/gpio.h>
this should be <asm/gpio.h>
> +static struct watchdog_info ident = {
> + .firmware_version = 0,
no point in setting this i dont think ...
> +static int gpio_wdt_ioctl(struct inode *inode, struct file *file,
> + unsigned int cmd, unsigned long arg)
> +{
> + case WDIOC_KEEPALIVE:
> + gpio_wdt_keepalive(wdt);
> + return 0;
this two lines should be merged.
> + default:
> + return -ENOIOCTLCMD;
should be -ENOTTY like all the other watchdogs
> +static char banner[] __initdata = KERN_INFO PFX "fixed %d.%03d seconds timeout (nowayout= %d)\n";
this only gets used once in the init function ... having it be broken
out like this is kind of silly
> +static int __init gpio_wdt_probe(struct platform_device *pdev)
shouldnt this be __devinit ?
> + if (watchdog) {
> + printk(KERN_ERR PFX "only one device supported\n");
> + return -ENODEV;
> + }
why ? it'd be trivial to abstract this driver away from a global
"watchdog" state into a proper arbitrary # of gpio watchdogs.
> +static int __exit gpio_wdt_remove(struct platform_device *pdev)
shouldnt this be __devexit ?
> +#define gpio_wdt_suspend()
> +#define gpio_wdt_resume()
i'm pretty sure this is incorrect and instead should read:
#define gpio_wdt_suspend NULL
#define gpio_wdt_resume NULL
> +static struct platform_driver gpio_wdt_driver = {
> + .probe = gpio_wdt_probe,
> + .remove = __exit_p(gpio_wdt_remove),
once you fix the remove to be __devexit, this should be __devexit_p()
> --- a/drivers/watchdog/Kconfig 2008-01-08 17:30:30.000000000 +0100
> +++ b/drivers/watchdog/Kconfig 2008-01-10 14:00:30.000000000 +0100
> +config GPIO_WATCHDOG
> + tristate "Support for GPIO connected watchdog"
> + depends on GENERIC_GPIO
> + help
> + This option enables support for a MAX823-like watchdog connected to
> + GPIO input/output.
this is misleading/confusing. suggested rewording:
This option enables support for a watchdog connected via GPIO lines
(such as the MAX823).
To compile this driver as a module, choose M here: the module will be
called gpio_wdt.
> --- a/include/linux/gpio_wdt.h 1970-01-01 01:00:00.000000000 +0100
> +++ b/include/linux/gpio_wdt.h 2008-01-10 15:48:42.000000000 +0100
> +#include <asm/arch/hardware.h>
no idea why you're including this as it is surely incorrect and not
needed. please remove it and add <linux/types.h>.
> +#define GPIO_WDT_DRIVER_NAME "gpio_wdt"
this belongs in the driver C file, not the header.
-mike
On Jan 11, 2008 11:40 AM, Florian Fainelli
<[email protected]> wrote:
> Did you look into hooking into Wim's uniform watchdog driver :
>
> http://git.kernel.org/?p=linux/kernel/git/wim/linux-2.6-watchdog-experimental.git;a=commit;h=732c54027e6c866f98857c4a6d1c6c466459dcd5
>
> Maybe you can save some code ?
seems like it'd be pointless for Marc to look at using this if it isnt
slated for merging into mainline anytime soon.
that said, i'd love to see a watchdog "core" as it pains me to see how
80% of all watchdogs are pretty much copy & paste & variable/function
renaming.
-mike
> > #include <linux/gpio_wdt.h>
>
> perhaps "watchdog" rather than "wdt" considering it's already
> "linux/watchdog.h" ?
or _wdt/wdt_ like the rest of the watchdog code uses for watchdog names
(wdt ->watchdog timer).
>
> > + case WDIOC_KEEPALIVE:
> > + gpio_wdt_keepalive(wdt);
> > + return 0;
>
> this two lines should be merged.
No.
> > + default:
> > + return -ENOIOCTLCMD;
>
> should be -ENOTTY like all the other watchdogs
Yes. That's a common confusion. -ENOIOCTLCMD is a magic code used
internally by some mid layers to indicate the driver doesn't know the
ioctl so use default behaviour. -ENOTTY (confusingly but this is Unix
history) is the right answer for unknowns.
>
> > +static char banner[] __initdata = KERN_INFO PFX "fixed %d.%03d seconds timeout (nowayout= %d)\n";
>
> this only gets used once in the init function ... having it be broken
> out like this is kind of silly
Saves memory - you can't make inlined strings __initdata without breaking
some compilers. So it is correct.
>
> > +static int __init gpio_wdt_probe(struct platform_device *pdev)
>
> shouldnt this be __devinit ?
IFF the device can be found/removed dynamically.
> > + if (watchdog) {
> > + printk(KERN_ERR PFX "only one device supported\n");
> > + return -ENODEV;
> > + }
>
> why ? it'd be trivial to abstract this driver away from a global
> "watchdog" state into a proper arbitrary # of gpio watchdogs.
The core watchdog code only supports one watchdog currently. This again
is correct.
On Jan 14, 2008 4:03 AM, Alan Cox <[email protected]> wrote:
> > > +static char banner[] __initdata = KERN_INFO PFX "fixed %d.%03d seconds timeout (nowayout= %d)\n";
> >
> > this only gets used once in the init function ... having it be broken
> > out like this is kind of silly
>
> Saves memory - you can't make inlined strings __initdata without breaking
> some compilers. So it is correct.
you could make the same argument about all strings used in all __init
functions. making a special case for this one string is just
confusing. this is also used from the *platfrom driver probe*
function, not the *module init* function, which means it should be
__devinitdata (see below) ... which quickly adds to the
confusion/craziness.
> > > +static int __init gpio_wdt_probe(struct platform_device *pdev)
> >
> > shouldnt this be __devinit ?
>
> IFF the device can be found/removed dynamically.
wont __init get freed once the module has finished loading ? he's
writing a platform driver which means the resources are usually
static, but there's no hard requirement that would prevent loading a
small module that merely contains additional platform resources. if
those resources declared a gpio watchdog, then i imagine things would
fall apart since the probe function for the registered platform driver
no longer exists. i also dont think there's a hard logical guarantee
that the the probing step will happen after the call to the platform
device register and before the kernel finishes loading the module (and
thus frees __init resources). so logically, using __init/__exit for
platform probe/remove drivers is plain wrong and it works here "by
accident".
> > > + if (watchdog) {
> > > + printk(KERN_ERR PFX "only one device supported\n");
> > > + return -ENODEV;
> > > + }
> >
> > why ? it'd be trivial to abstract this driver away from a global
> > "watchdog" state into a proper arbitrary # of gpio watchdogs.
>
> The core watchdog code only supports one watchdog currently. This again
> is correct.
today you're of course correct. but if we're looking to unify
watchdogs, keeping the 1 watchdog limit seems silly. and considering
the trivial amount of effort to move these resources to the private
data pointers ... might as well get the work done now while it's fresh
in his mind. his call of course though.
-mike
> > Saves memory - you can't make inlined strings __initdata without breaking
> > some compilers. So it is correct.
>
> you could make the same argument about all strings used in all __init
> functions. making a special case for this one string is just
> confusing. this is also used from the *platfrom driver probe*
> function, not the *module init* function, which means it should be
> __devinitdata (see below) ... which quickly adds to the
> confusion/craziness.
Not neccessarily see below. For strings there is a tricky tradeoff
between space on embedded boxes and simplicity. On a 2GB desktop PC the
decision is fairly trivial, on a PDA it gets a bit more important.
> > > > +static int __init gpio_wdt_probe(struct platform_device *pdev)
> > >
> > > shouldnt this be __devinit ?
> >
> > IFF the device can be found/removed dynamically.
>
> wont __init get freed once the module has finished loading ?
If your platform creates the device statically (as a lot of embedded
platforms do) then the __init is fine. The platform register in
init_module will call back the driver probe method and attach the device
before the init_module exits.
Alan
On Jan 14, 2008 4:29 AM, Alan Cox <[email protected]> wrote:
> > > Saves memory - you can't make inlined strings __initdata without breaking
> > > some compilers. So it is correct.
> >
> > you could make the same argument about all strings used in all __init
> > functions. making a special case for this one string is just
> > confusing. this is also used from the *platfrom driver probe*
> > function, not the *module init* function, which means it should be
> > __devinitdata (see below) ... which quickly adds to the
> > confusion/craziness.
>
> Not neccessarily see below. For strings there is a tricky tradeoff
> between space on embedded boxes and simplicity. On a 2GB desktop PC the
> decision is fairly trivial, on a PDA it gets a bit more important.
i'm aware of this (i bang on embedded no-mmu systems). if his banner
string was crazy long, i'd more understand, but it isnt any longer
than any one of his error strings in the probe function. in fact, all
of the error strings combined are more than 3x the size of his banner.
wonder if we could design a printk designed for __init functions to
address this in a clean fashion.
#define init_printk(fmt, __VA_ARGS__) \
do { \
static const __init char __fmt[] = fmt; \
printk(__fmt , ## __VA_ARGS__); \
} while (0)
(yes, i know this isnt perfect as you'd need to pass back the return
value of printk(), but it's an idea)
> > > > > +static int __init gpio_wdt_probe(struct platform_device *pdev)
> > > >
> > > > shouldnt this be __devinit ?
> > >
> > > IFF the device can be found/removed dynamically.
> >
> > wont __init get freed once the module has finished loading ?
>
> If your platform creates the device statically (as a lot of embedded
> platforms do) then the __init is fine. The platform register in
> init_module will call back the driver probe method and attach the device
> before the init_module exits.
there is no hard requirement anywhere that says platform resources
must be in the board resources file. marking the functions as __init
instead of __devinit will basically cause a kernel crash if someone
tries to use dynamic platform resources. there is no option that i'm
aware of that prevents dynamic platform resources which means there is
no way for the driver to say "i wont work with standard dynamic
platform resources".
-mike
On Mon, 14 Jan 2008 04:45:25 -0500
"Mike Frysinger" <[email protected]> wrote:
> there is no hard requirement anywhere that says platform resources
> must be in the board resources file. marking the functions as __init
> instead of __devinit will basically cause a kernel crash if someone
> tries to use dynamic platform resources. there is no option that i'm
> aware of that prevents dynamic platform resources which means there is
> no way for the driver to say "i wont work with standard dynamic
> platform resources".
There is: platform_driver_probe(). It takes the probe function as a
parameter so that it can be left out of the platform_driver struct.
After it returns, there are no references to the probe function left
around, so if you call platform_driver_probe() instead of
platform_driver_register(), the probe function can be __init.
I agree that the driver is not safe in its current form.
Haavard
On Jan 14, 2008 7:14 AM, Haavard Skinnemoen <[email protected]> wrote:
> "Mike Frysinger" <[email protected]> wrote:
> > there is no hard requirement anywhere that says platform resources
> > must be in the board resources file. marking the functions as __init
> > instead of __devinit will basically cause a kernel crash if someone
> > tries to use dynamic platform resources. there is no option that i'm
> > aware of that prevents dynamic platform resources which means there is
> > no way for the driver to say "i wont work with standard dynamic
> > platform resources".
>
> There is: platform_driver_probe(). It takes the probe function as a
> parameter so that it can be left out of the platform_driver struct.
> After it returns, there are no references to the probe function left
> around, so if you call platform_driver_probe() instead of
> platform_driver_register(), the probe function can be __init.
ah, thanks for that. i think in that case, there's no way to bind the
release function to the process ? which means the driver is no longer
unloadable ? which means the platform driver release function can be
scrubbed as well as the module exit function as well as changing the
Kconfig to be a bool ?
-mike
Hi,
"Mike Frysinger" <[email protected]> writes:
> wonder if we could design a printk designed for __init functions to
> address this in a clean fashion.
> #define init_printk(fmt, __VA_ARGS__) \
> do { \
> static const __init char __fmt[] = fmt; \
> printk(__fmt , ## __VA_ARGS__); \
> } while (0)
>
> (yes, i know this isnt perfect as you'd need to pass back the return
> value of printk(), but it's an idea)
How about:
#define init_printk(fmt, args...) ({ \
static const __init char __fmt[] = fmt; \
printk(__fmt, args); \
})
Now it returns the printk result.
Hannes
On Jan 14, 2008 7:49 AM, Johannes Weiner <[email protected]> wrote:
> "Mike Frysinger" <[email protected]> writes:
> > wonder if we could design a printk designed for __init functions to
> > address this in a clean fashion.
> > #define init_printk(fmt, __VA_ARGS__) \
> > do { \
> > static const __init char __fmt[] = fmt; \
> > printk(__fmt , ## __VA_ARGS__); \
> > } while (0)
> >
> > (yes, i know this isnt perfect as you'd need to pass back the return
> > value of printk(), but it's an idea)
>
> How about:
>
> #define init_printk(fmt, args...) ({ \
> static const __init char __fmt[] = fmt; \
> printk(__fmt, args); \
> })
>
> Now it returns the printk result.
i wasnt really worried about that ... i was worried about other random
things i may have missed
your dropping of ## wont work as you need gcc to expand args and take
away the , in the simple 1 arg case:
init_printk("MOO");
-mike
On Mon, 14 Jan 2008 07:22:39 -0500
"Mike Frysinger" <[email protected]> wrote:
> > There is: platform_driver_probe(). It takes the probe function as a
> > parameter so that it can be left out of the platform_driver struct.
> > After it returns, there are no references to the probe function left
> > around, so if you call platform_driver_probe() instead of
> > platform_driver_register(), the probe function can be __init.
>
> ah, thanks for that. i think in that case, there's no way to bind the
> release function to the process ? which means the driver is no longer
> unloadable ? which means the platform driver release function can be
> scrubbed as well as the module exit function as well as changing the
> Kconfig to be a bool ?
No, you can still provide a remove() callback, but it can usually be
__exit_p since it will only be needed if you compile the driver as a
module.
The platform_driver will be registered just as before; the only
difference is that it won't have a probe() callback so dynamically
added devices can't be bound to the driver. If the driver has a
remove() callback, it will be called when the driver is unregistered,
but that will usually only happen if the driver is compiled as a
module (and has a module exit function.)
Haavard
Hi,
"Mike Frysinger" <[email protected]> writes:
>> How about:
>>
>> #define init_printk(fmt, args...) ({ \
>> static const __init char __fmt[] = fmt; \
>> printk(__fmt, args); \
>> })
>>
>> Now it returns the printk result.
>
> i wasnt really worried about that ... i was worried about other random
> things i may have missed
Ok.
> your dropping of ## wont work as you need gcc to expand args and take
> away the , in the simple 1 arg case:
> init_printk("MOO");
Whoops, totally oversaw this one. Of course, this must be in.
Perhaps an even more generic solution would be good, like this:
#define section_printk(sect, fmt, args...) ({ \
static const sect char __fmt[] = fmt; \
printk(__fmt, ## args); \
})
And then just have convenience wrappers like {,dev}init_printk, ... etc.
Can someone please shout at us if there is a fundamental problem with
this approach?
Hannes