Re: [RFC] Watchdog device class

[Rudolf Marek]

Hello Alan,

>>The char device of watchdog class is compatible with existing watchdog API,
>>so no need to change the user applications. There is just one exception
>>and this is temperature handling. I belive it should be implemented not
>>via IOCTL but using the HWMON class. (100% compatibility can be restored
>>with the ioctl class op)
> 
> 
> Then it should be kept.

Ok I think best would be to create ops->ioctl callback for not so common
ioctls (like the temp one) and let the driver to deal with them.

> The watchdog API simply pre-dates the sysfs world, it goes back to the
> 1.0-1.2 era and has remained very consistent since that time.
> 
> If you expose it in sysfs somewhere (which I think is a good idea) then
> the units should probably also be fixed in the sysfs case to be metric
> (ie Kelvin or Centigrade float values) [or scaled int]

Yep I have this in mind. But not yet the topic of the day. So far I have:

name - this is not need perhaps it can be found in another place in sysfs
timeout - timeout value in sec - convert it to ms perhaps?
ping - "ping" file, to replace the /dev/watchdog writes
boot_status - the boot status - meaning same as IOCTL has, generaly the reset reason
status - current status - same as ioctl equivalent

And optional firmware_ver to reflect the IOCTL equvalent.

This is needed to be discussed with Wim first. I hope he will speak up ;)

As for the temps/fans I belive the driver should register in hwmon class and use
hwmon class sysfs iterface and then just create sort of relation between the
sysfs files/classes, so the watchdog app can find the temps.

>>	int (*set_timeout)(struct device *, int sec);
> 
> 
> Pass the usual time structures instead. Seconds is a field so it is free
> but it means all the signed/unsigned stuff and any future subsecond
> watchdogs for embedded environments don't break stuff.

Good idea, I will change it. Thanks.

>>	int (*notify_reboot)(struct notifier_block *this, unsigned long code,
>>	        void *unused);
> Can this not use the power management callbacks from the device model
> instead

Ah I knew about the suspend/resume and it seems there is also a shutdown one.

 >>	/* this may be removed in the future */
>>	struct watchdog_info legacy_info;
> 
> This wants breaking out into sysfs, but again the ioctls are expected
> and standardised for years now.
> 
> People have talked about sorting out a watchdog helper library for years
> so this is overdue, and doing it with the class model in mind is even
> better.

I was quite amazed when I saw same code copied 40x in one directory ;)

Thanks for the comments,
Regards
Rudolf
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/