Re: [PATCH 01/11] RTC subsystem, class

From: Alessandro Zummo
Date: Wed Feb 22 2006 - 20:08:36 EST


On Wed, 22 Feb 2006 14:15:54 -0800
Andrew Morton <akpm@xxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/