Hello John,
I have been working on a new RTC driver for some custom RTC device
lately. It has the usual features like AIE, UIE and PIE.
I cloned the rtc-test.c driver as Documentation/rtc.txt suggested and
noticed that my driver would work with the test application even though
some of the driver functions were not implemented at all...
So I found out about your recent changes with regard to "RTC
virtualisation" and I understood the steps you have taken. It's not
necessary to provide all functionality through the RTC driver directly
any more, because some of the stuff is emulated via hrtimers.
One problem is that the documentation and the code have gotten out of
sync. People like me, who dive into the RTC subsystem for the first
time, can be easily fooled now.
So the main question now is what the minimal required functionality for
a RTC driver now actually is. Can you give me some hints?
I think it would be best if the rtc-test.c driver could be updated to
have a template new drivers can be cloned from.
Another thing that catched my eye is the current use of the read_alarm()
member in struct rtc_class_ops in rtc_read_alarm() in
drivers/rtc/interface.c. This is the only place where the member is used
and it's only used as a boolean there. The driver specific read_alarm()
op is never actually called, so I think that op can be transformed into
a boolean and all the read_alarm() functions from all the rtc drivers
can be removed - that's quite a lot of a code.
Best regards
Michael.
On Mon, 2011-04-04 at 08:55 +0200, Michael Hunold wrote:
> I have been working on a new RTC driver for some custom RTC device
> lately. It has the usual features like AIE, UIE and PIE.
>
> I cloned the rtc-test.c driver as Documentation/rtc.txt suggested and
> noticed that my driver would work with the test application even though
> some of the driver functions were not implemented at all...
>
> So I found out about your recent changes with regard to "RTC
> virtualisation" and I understood the steps you have taken. It's not
> necessary to provide all functionality through the RTC driver directly
> any more, because some of the stuff is emulated via hrtimers.
>
> One problem is that the documentation and the code have gotten out of
> sync. People like me, who dive into the RTC subsystem for the first
> time, can be easily fooled now.
So I have committed some cleanups to the documentation already in
2.6.39-rc1, but I'd be greatly interested in examples of where you found
it confusing.
> So the main question now is what the minimal required functionality for
> a RTC driver now actually is. Can you give me some hints?
Well, it depends on the functionality that is available. Completely
minimal might be just clock-only functions:
int (*read_time)(struct device *, struct rtc_time *);
int (*set_time)(struct device *, struct rtc_time *);
or
int (*set_mmss)(struct device *, unsigned long secs);
Then to get interrupt/alarm functionality:
int (*alarm_irq_enable)(struct device *, unsigned int enabled);
int (*set_alarm)(struct device *, struct rtc_wkalrm *);
To have persistence over reboots:
int (*read_alarm)(struct device *, struct rtc_wkalrm *);
These are completely optional:
int (*proc)(struct device *, struct seq_file *);
int (*ioctl)(struct device *, unsigned int, unsigned long);
int (*open)(struct device *);
void (*release)(struct device *);
int (*read_callback)(struct device *, int data);
> I think it would be best if the rtc-test.c driver could be updated to
> have a template new drivers can be cloned from.
So upstream in 2.6.39-rc1 I think this has been done, but please let me
know if there are any improvements you see that could be made.
> Another thing that catched my eye is the current use of the read_alarm()
> member in struct rtc_class_ops in rtc_read_alarm() in
> drivers/rtc/interface.c. This is the only place where the member is used
> and it's only used as a boolean there. The driver specific read_alarm()
> op is never actually called, so I think that op can be transformed into
> a boolean and all the read_alarm() functions from all the rtc drivers
> can be removed - that's quite a lot of a code.
Yes, I actually tried to remove it, but realized we need it to restore
the aie_timer state at boot.
Please check 2.6.39-rc1, and let me know if your concerns have not been
addressed already?
thanks
-john