Messages in this thread | | | Date | Wed, 22 Jul 2015 14:44:46 -0700 | From | Dmitry Torokhov <> | Subject | Re: [rtc-linux] [PATCH 4/4] RTC: switch to using is_visible() to control sysfs attributes |
| |
On Wed, Jul 22, 2015 at 10:57:35PM +0200, Alexandre Belloni wrote: > (Krzysztof, be careful, Dmitry was not in copy of your maili, you should > probably check your mailer config) > > On 21/07/2015 at 10:21:11 +0900, Krzysztof Kozlowski wrote : > > 2015-07-21 8:02 GMT+09:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>: > > > static ssize_t > > > -rtc_sysfs_set_wakealarm(struct device *dev, struct device_attribute *attr, > > > +wakealarm_store(struct device *dev, struct device_attribute *attr, > > > const char *buf, size_t n) > > > { > > > ssize_t retval; > > > @@ -221,45 +209,58 @@ rtc_sysfs_set_wakealarm(struct device *dev, struct device_attribute *attr, > > > retval = rtc_set_alarm(rtc, &alm); > > > return (retval < 0) ? retval : n; > > > } > > > -static DEVICE_ATTR(wakealarm, S_IRUGO | S_IWUSR, > > > - rtc_sysfs_show_wakealarm, rtc_sysfs_set_wakealarm); > > > +static DEVICE_ATTR_RW(wakealarm); > > > > This and renaming of show/store functions look unrelated > > > > I don't really mind that one but I would also prefer if it could be > separated.
OK, I will.
> > > > > > > +static struct attribute *rtc_attrs[] = { > > > + &dev_attr_name.attr, > > > + &dev_attr_date.attr, > > > + &dev_attr_time.attr, > > > + &dev_attr_since_epoch.attr, > > > + &dev_attr_max_user_freq.attr, > > > + &dev_attr_hctosys.attr, > > > + &dev_attr_wakealarm.attr, > > > + NULL, > > > +}; > > > > > > -/* The reason to trigger an alarm with no process watching it (via sysfs) > > > +/* > > > + * The reason to trigger an alarm with no process watching it (via sysfs) > > > * is its side effect: waking from a system state like suspend-to-RAM or > > > * suspend-to-disk. So: no attribute unless that side effect is possible. > > > * (Userspace may disable that mechanism later.) > > > */ > > > -static inline int rtc_does_wakealarm(struct rtc_device *rtc) > > > +static bool rtc_does_wakealarm(struct rtc_device *rtc) > > > { > > > if (!device_can_wakeup(rtc->dev.parent)) > > > - return 0; > > > + return false; > > > + > > > return rtc->ops->set_alarm != NULL; > > > } > > > > This looks unrelated too and confuses me. Could you split such cleanup > > from main goal of the patch? > > > > That one is bothering me too.
Will separate this too.
Thanks.
-- Dmitry
| |