From: Alessandro Zummo <alessandro.zummo@towertech.it>
To: Andrew Morton <akpm@osdl.org>
Cc: Alessandro Zummo <a.zummo@towertech.it>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 01/11] RTC subsystem, class
Date: Thu, 23 Feb 2006 02:09:52 +0100 [thread overview]
Message-ID: <20060223020952.42124378@inspiron> (raw)
In-Reply-To: <20060222141554.0f5e2aa3.akpm@osdl.org>
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
next prev parent reply other threads:[~2006-02-23 1:10 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-02-19 23:22 [PATCH 00/11] RTC subsystem Alessandro Zummo
2006-02-19 23:22 ` [PATCH 01/11] RTC subsystem, class Alessandro Zummo
2006-02-22 22:15 ` Andrew Morton
2006-02-23 1:09 ` Alessandro Zummo [this message]
2006-02-23 1:31 ` Andrew Morton
2006-02-19 23:22 ` [PATCH 02/11] RTC subsystem, ARM cleanup Alessandro Zummo
2006-02-19 23:22 ` [PATCH 03/11] RTC subsystem, I2C cleanup Alessandro Zummo
2006-02-19 23:22 ` [PATCH 04/11] RTC subsystem, sysfs interface Alessandro Zummo
2006-02-19 23:22 ` [PATCH 05/11] RTC subsystem, proc interface Alessandro Zummo
2006-02-19 23:22 ` [PATCH 06/11] RTC subsystem, dev interface Alessandro Zummo
2006-02-19 23:22 ` [PATCH 07/11] RTC subsystem, X1205 driver Alessandro Zummo
2006-02-19 23:22 ` [PATCH 08/11] RTC subsystem, test device/driver Alessandro Zummo
2006-02-19 23:22 ` [PATCH 09/11] RTC subsystem, DS1672 driver Alessandro Zummo
2006-02-19 23:22 ` [PATCH 10/11] RTC subsystem, PCF8563 driver Alessandro Zummo
2006-02-19 23:22 ` [PATCH 11/11] RTC subsystem, RS5C372 driver Alessandro Zummo
2006-02-20 0:58 ` [PATCH 00/11] RTC subsystem Andrew Morton
2006-02-20 1:10 ` Alessandro Zummo
[not found] <5FUPV-4r4-3@gated-at.bofh.it>
[not found] ` <5FUZC-4Rr-17@gated-at.bofh.it>
2006-02-14 8:08 ` [PATCH 01/11] RTC subsystem, class Thomas Petazzoni
-- strict thread matches above, loose matches on Subject: below --
2006-02-13 22:54 [PATCH 00/11] RTC subsystem Alessandro Zummo
2006-02-13 22:54 ` [PATCH 01/11] RTC subsystem, class Alessandro Zummo
2006-02-14 3:49 ` Dmitry Torokhov
2006-02-15 0:24 ` Alessandro Zummo
2006-02-14 9:02 ` Paul Mundt
2006-02-14 10:07 ` Alessandro Zummo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20060223020952.42124378@inspiron \
--to=alessandro.zummo@towertech.it \
--cc=a.zummo@towertech.it \
--cc=akpm@osdl.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).