Re: [RFC, PATCH] watchdog on gpio

["Mike Frysinger" <vapier.adi@gmail.com>]

On Jan 14, 2008 4:03 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> 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