Hi Maurizio,
> this is a report about the current status of the rtc branch.
(Note: The branch is located at lp:rtc-helenos.)
I went through your changes. Below you can find some new and old
comments to the code. The comments are concluded by some DDF related
doubts that I have but unfortunately cannot answer myself. It would
therefore be appreciated if someone more familiar with DDF could help to
answer them.
Besides of the issues in the comments, I found that after cleanly
merging this into mainline, make check fails. At least on arm32, your
new code seems to be bringing in a dependency on some undefined symbol:
../../lib/c/libc.a(time.o): In function `difftime':
/home/jermar/software/HelenOS.mainline/uspace/lib/c/generic/time.c:981:
undefined reference to `__aeabi_i2d'
Jakub's comments:
=================
+ /** time at which the system booted */
+ time_t boottime;
+} rtc_t;
Ok, now that boottime is part of rtc_t and that the driver supports
device removal, wouldn't it make more sense to call this something like
online_time or attach_time?
+static bool
+rtc_pio_enable(rtc_t *rtc)
+{
+ if (pio_enable((void *)(uintptr_t) rtc->io_addr, REG_COUNT,
+ (void **) &rtc->port)) {
I believe both typecasts of io_addr are now superfluous.
+rtc_register_read(rtc_t *rtc, int reg)
+{
+ pio_write_8(rtc->port, reg);
+ return pio_read_8(rtc->port + 1);
+}
Here it would be nice to get rid of the magical + 1 in favour of some
preprocessor macro.
+rtc_register_write(rtc_t *rtc, int reg, int data)
+{
+ pio_write_8(rtc->port, reg);
+ pio_write_8(rtc->port + 1, data);
+}
Same as above.
+static void remote_clock_time_set(ddf_fun_t *fun, void *ops,
+ ipc_callid_t callid, ipc_call_t *call)
+{
...
+ if (!clock_dev_ops->time_set) {
+ /* The driver does not support the time_set()
functionality */
+ async_data_write_finalize(cid, NULL, 0);
+ async_answer_0(callid, ENOTSUP);
+ return;
+ }
In this and similar cases, the error path is not supposed to call
async_data_write_finalize(). Use async_answer() to return some error
code instead.
+ /* Check the battery status (if present) */
+ async_exch_t *exch = async_exchange_begin(sess);
+ rc = async_req_0_1(exch, CLOCK_GET_BATTERY_STATUS, &battery_ok);
+ async_exchange_end(exch);
+
+ if (rc == EOK && !battery_ok)
+ printf(NAME ": Warning! RTC battery dead\n");
Testing the battery status is a nice demo of the rtc-cmos
driver abilities, but IMHO is a bit out of scope for the date command.
My first thought when I learned about CLOG_GET_BATTERY_STATUS was that
the driver should perhaps have another interface that would be used by
all devices with some battery and that would enable querying the battery
status in a unified way.
+static int
+rtc_dev_remove(ddf_dev_t *dev)
+{
...
+ rtc->removed = true;
+static int
+rtc_open(ddf_fun_t *fun)
+{
...
+ if (rtc->removed)
+ rc = ENXIO;
+ else {
+ rc = EOK;
+ rtc->clients_connected++;
I am not really sure about this one, but is this really needed? I'd
assume that a sound device driver framework would not allow you to open
a non-existing device. Likewise, I'd assume that the softstate gets
destructed on dev_remove(). The last doubt I have is whether the driver
is not duplicating some work when maintaining the count of connected
clients. The motivation seems to be to prevent device deletion when
someone is connected. Can someone sched some light into this? Jiri? Jano?
Thanks,
Jakub
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
