On Wed, 22 Feb 2006 14:15:54 -0800
Andrew Morton <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
