Messages in this thread | | | Date | Thu, 23 Feb 2006 02:09:52 +0100 | From | Alessandro Zummo <> | Subject | Re: [PATCH 01/11] RTC subsystem, class |
| |
On Wed, 22 Feb 2006 14:15:54 -0800 Andrew Morton <akpm@osdl.org> wrote:
> Couple of questions. > > a) Is all this code 100% compatible with existing kernel interfaces? No > userspace-visible breakage? Right down the all the same -EFOO return > codes for all the same errors?
this new class works only in places of the kernel were there were no RTCs before.. basically, I haven't touched the x86 or any other platform clock and focused only on i2c clocks which were actually not used.
From the userspace point of view, the interface is the same. I noticed that hwclock does not like things like time that is not changing or an -EINVAL on a read, which can instead happen when you use i2c clocks. However, that is not an issue yet because hwclock has not been used on those i2c clocks until now.
> > b) Will the kernel compile and run at each stage of your patch series? > I hit a nasty no-compile half an hour through a git-bisect session > yesterday and it's still smarting.
I' haven't tested it.. I think there may be problems because at some points some functions in the kernel needs to be renamed and/or changed.. i would say the first two patches should be applied together.
> Code looks nice and clean.
thanks.
> > + if ((rtc = kzalloc(sizeof(struct rtc_device), GFP_KERNEL)) == NULL) { > > You do a lot of > > if ((lhs = rhs) == something) > > But preferred kernel style is > > lhs = rhs; > if (lhs == something)
good to know, that is also my preferred style. I will happily change this, I just thought the kernel style was the other one :)
> Generally, kernel style is to keep things as utterly simple as they can be.
[..]
> > +config RTC_HCTOSYS_DEVICE > > + string "The RTC to read the time from" > > + depends on RTC_HCTOSYS = y > > + default "rtc0" > > + help > > + The RTC device that will be used as the source for > > + the system time, usually rtc0. > > hm. Doesn't the above disable RTC_HCTOSYS and RTC_HCTOSYS_DEVICE if > RTC_CLASS=m?
yes. I can't remember if it is intended, but the point of having hctosys was to copy the time early in the bootup process.
> > + > > +extern struct class *rtc_class; > > Please always put extern declarations in a header file.
ack.
> > +EXPORT_SYMBOL(rtc_update_irq); > > I don't know what this does. > > Please document all non-static functions. Preferably with kernel-doc > format. Feel free to document static functions too..
will do.
> > +int rtc_irq_set_freq(struct class_device *class_dev, struct rtc_task *task, int freq) > > +{ > > + int err = 0, tmp = 0; > > + unsigned long flags; > > + struct rtc_device *rtc = to_rtc_device(class_dev); > > + > > + /* allowed range is 2-8192 */ > > + if (freq < 2 || freq > 8192) > > + return -EINVAL; > > + > > +/* if ((freq > rtc_max_user_freq) && (!capable(CAP_SYS_RESOURCE))) > > + return -EACCES; > > +*/ > > What happened to rtc_max_user_freq?
not implemented yet, I need to handle it in a different way. rtc_irq_set_freq is the kernel interface, I must move this check in the /dev/rtc code.
How can I handle further updates, just repost the whole patchset to lkml ?
thanks for you review!
--
Best regards,
Alessandro Zummo, Tower Technologies - Turin, Italy
http://www.towertech.it
- 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/
| |