Messages in this thread | | | Date | Tue, 18 Apr 2006 21:32:06 +0200 | From | Rudolf Marek <> | Subject | Re: [RFC] Watchdog device class |
| |
Hello Arnd,
> Wim used to have an experimental tree with a similar idea. Did you look > at that before you started this? Is that tree still maintained?
No I did not, I thought if the class exist it is already there ... Instead I have taken the RTC class which was fairly similar to the watchdog needs and changed it to a watchdog one. I think this may answer some of your questions ;)
Now we have something that works and might be used as the base for the watchdog class. Many thanks for your comments.
>>+#include <linux/module.h> >>+#include "watchdog.h" >>+ >>+static struct class *watchdog_dev_class; >>+static dev_t watchdog_devt; >>+ >>+#define WATCHDOG_DEV_MAX 8 /* 8 watchdogs should be enough for everyone... */ > > What's the point in having more than one watchdog active? > If you want more than one, why hardcode a specific limit?
I thought there might be such future need, nowdays it used to test the stuff.
If nobody here wants it I have no problem to change the class just to allow one active watchdog.
> > Existing user space typically depends on the watchdog device being called > /dev/watchdog, which breaks if your devices are called watchdog%d. > I can see two ways out of this: > > 1. add a udev rule that creates /dev/watchdog as an alias for /dev/watchdog0. > 2. add a misc device /dev/watchdog that works as a multiplexer for all others, > similar to the idea of /dev/input/mice as a multiplexer for /dev/input/mouse%d.
True, this depends if someone wants this multi watchdog stuff.
>>+#define WATCHDOG_DEVICE_NAME_SIZE 20 >>+ >>+struct watchdog_device { >>+ struct class_device class_dev; >>+ struct module *owner; >>+ >>+ int id; >>+ char name[WATCHDOG_DEVICE_NAME_SIZE]; > > Can this be the embedded name in class_device?
I copied this from the RTC, and because it is already in kernel I assumed it is OK to have it this way.
>>+ >>+ struct watchdog_class_ops *ops; >>+ struct mutex ops_lock; >>+ >>+ struct class_device *watchdog_dev; > > > Having two classes (watchdog_class and watchdog_dev_class) as well as > two class_devices (class_dev and watchdog_dev) per device seems wrong. > What is the reason for splitting these?
Again, this was taken from the RTC subsystem and I left the modularity of the stuff. But for the watchdog case it might go without the additional class. As for the watchdog_dev it depends how tight we want to integrate it into the class.
>>+/* watchdog_device_register_simple, will register device into watchdog class >>+ just with default timeout from kernel configuration. >>+ >>+ watchdog_device_register_selfping, will register the watchdog device same >>+ way as above function, but the device will be pinged every selfping interval >>+ (useful for watchdog with damm fast timeouts) >>+*/ >>+ >>+#define watchdog_device_register_simple(name, dev, ops) \ >>+ _watchdog_device_register(name, dev, ops, THIS_MODULE, CONFIG_WATCHDOG_DEFAULT_TIMEOUT, 0) >>+ >>+#define watchdog_device_register_selfping(name, dev, ops, selfping) \ >>+ _watchdog_device_register(name, dev, ops, THIS_MODULE, CONFIG_WATCHDOG_DEFAULT_TIMEOUT, selfping) > > > All callers of this seem to have constant arguments (except dev), so you > could easily put name, owner, timeout and selfping into the ops structure. > Maybe the structure could use a different name then.
Hmm even to think the name is a bit problem, because the in it will be used for two different things, for the ops and as the data container.
>>+static ssize_t watchdog_sysfs_show_name(struct class_device *dev, char *buf) >>+{ >>+ return sprintf(buf, "%s\n", to_watchdog_device(dev)->name); >>+} >>+static CLASS_DEVICE_ATTR(name, S_IRUGO, watchdog_sysfs_show_name, NULL); > > > Why do you need a name attribute? Should the name of the class_device > itself not be enough as an identifier?
True, RTC has it this way too so I thought there must be some reason...
> What's the point in making the sysfs stuff an extra module? I guess it > would be a lot simpler to just always add the files. We might want to > make the dev interface optional, but then again it is what all applications > to date are using, so you may just as well have a single base module > and no infrastructure for multiple interfaces at all.
Well the sysfs seemed still bit a future for watchdog and embedded devices might want to save some space. I even did not know that sysfs interface will be on the topic so quickly. So this was the reason I left it as it was in RTC...
Thanks, Regards Rudolf
PS: please CC me. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |