Re: [RFC] watchdog: Add watchdog device control through sysfs attributes

[Pratyush Anand <panand@redhat.com>]

On 30/08/2015:11:39:38 AM, Guenter Roeck wrote:
> On 08/30/2015 11:17 AM, Pratyush Anand wrote:
> >Hi Guenter,
> >On 30/08/2015:07:58:27 AM, Guenter Roeck wrote:
> >>
> >>I can only see a single caller of watchdog_dev_register().
> >>Changing that one call should not be such a problem.
> >
> >Sure..Probably I was sleeping when I greped watchdog_dev_register :(
> >
> >>
> >>Also, you could name the groups variable something like wdc_class_dev_groups,
> >>make it public, and assign it in watchdog_core.c as follows.
> >
> >I will avoid making public whereever possible.
> >
> Your argument seems to be that it is better to introduce a new public
> single-use API function to avoid introducing a public variable.
> 
> Not sure if I understand the logic.

Sorry, if it was not clear..I agreed to change watchdog_dev_register instead of
introducing a new public variable.

> 
> >>
> >>static struct class watchdog_class = {
> >>	.name = "watchdog",
> >>	.owner = THIS_MODULE,
> >>	.dev_groups = wdc_class_dev_groups,
> >>};
> >>
> >>Then use class_register() and class_unregister() instead of class_create()
> >>and class_destroy(), and you would be done w/o any further API change.
> >>
> >>>>
> >>>>>+		return size;
> >>>>>+}
> >>>>>+
> >>>>>+static ssize_t timeout_show(struct device *dev,
> >>>>>+		struct device_attribute *attr, char *buf)
> >>>>>+{
> >>>>>+	struct watchdog_device *wdd = dev_get_drvdata(dev);
> >>>>>+	ssize_t status;
> >>>>>+
> >>>>>+	mutex_lock(&wdd->lock);
> >>>>>+	if (wdd->timeout == 0)
> >>>>>+		status = -EOPNOTSUPP;
> >>>>
> >>>>Why ?
> >>>
> >>>It has been copied from case WDIOC_GETTIMEOUT: which says:
> >>>timeout == 0 means that we don't know the timeout.
> >>>
> >>
> >>Just return 0 to mean "the timeout is unknown", and make it part of the
> >>sysfs ABI. Returning an error just makes user space code more complicated.
> >
> >Now the new code will check wdd->timeout == 0 in is_visible. So should be fine.
> >What do you say.?
> >
> timeout can be configured through ioctl, unless I am missing something,
> so I am not sure if I agree. It could still end up being 0.

Hummm..it did not allow to set timeout=0 for iTCO_wdt..but I can see that there
are some devices which sets min_timeout=0 and so your argument seems to be
correct and I will change as you suggested.
However, I am wondering should a device be allowed to set timeout=0. What
purpose will that serve?... may be for testing purpose can be used..

~Pratyush