Ladislav Michl wrote:
On Tue, Apr 12, 2005 at 07:10:55PM +0100, James Chapman wrote: [snip]
It is used by the Radstone ppc7d platform, arch/ppc/radstone_ppc7d.c but wasn't added until very recently (2.6.12-rc2 I think).
To be honest, I meant to remove the 'id' thing before submitting the driver. There's no need to support more than one of these devices.
Patch bellow remove ds1337_do_command function and things needed by it. I think device should be identified by bus and address as Jean said. Please let me know if that fits your needs.
I think you misunderstood what I meant by "remove the 'id' thing" (probably my fault). ds1337_do_command() is needed by ppc7d so don't remove it. I meant remove the id parameter from the call and change the ds1337 driver to support only one instance of the device.
I'm assuming that you want to use drivers/char/genrtc.c to access ds1337
from userspace, but in arch/ppc/platforms/radstone_ppc7d.c ppc_md.get_rtc_time used by genrtc via get_rtc_time in asm-ppc/rtc.h
is set to NULL (same for set_rtc_time) and I didn't find where (if)
ds1337 registers to ppc_md.get_rtc_time.
For ppc at least, it's the platform code that hooks up get_rtc_time().
Last time I looked in -mm, get_rtc_time() and set_rtc_time() were being set up in ppc7d to use this driver. I won't be able to check until the end of the week so please bear with me.
Functions in asm-ppc/rtc.h also do magic with tm_mon and tm_year so this driver doesn't need to handle epoch separately and doesn't need to be aware that tm_mon starts from zero...
I don't understand. What code in ds1337 is unneeded?
m68k, mips and parisc does the same in asm/rtc.h unlike arm, so I this driver probably won't work for me without some tweaks to arm code.
[snip]
Back to the issue, some random thoughts summarizing my opinion:
[snip]
3* Having the driver write an arbitrary non-0 value to the register should not be done unless the system has been identified. I have no idea how your system can be identified (DMI?), but if it can't, then I'd better see the register ignored altogether.
My board is OMAP (ARM core) based and there are ARM specific functions (if (machine_is_xxx()) do_something(); ), but it is not what you want to see in generic driver. It may be possible to use platform_data to pass information to driver, but I do not like this idea.
So, if we use entry in sysfs, then only root can write it and root is allowed to do weird things. Device itself refuses any action until high four bits are 0xa. If that is still not enough I just found this patch http://groups-beta.google.com/group/fa.linux.kernel/msg/06e0368f86c8f824 so you can use configfs to explicitly create "charge" entry. ( * I'm considering that an overkill * I'm not sure if it can be easily done with configfs)
I'd add config option (disabled by default) for "charge" entry, if you feel it is too dangerous. However I think that people should be a bit responsible for their actions and not writing any randoms values to any random files in /sys :)
4* Remember that you can always write a simple C tool relying on the i2c-dev interface to do the job. The advantage of this approach is that you can put big fat warnings and request user confirmation before any action.
This makes sense. Ladislav, would this work for you? I guess we'd still add code to the ds1337 driver to detect ds1339 in order to ensure that this tool could not modify register 0 of a ds1337 by accident?
Yes, that would definitely work for me and I'm fine with that in case proposal above would be rejected.
Ok. Jean, what do you think? Do we really want a "charge" sysfs entry? I don't have a strong opinion on this.
Remove nowhere referenced ds1337_do_command function. Apply after ds1337 patches 1-3.
Please don't apply this patch. I will modify the ds1337_do_command() API to remove the "id" parameter and fixup ppc7d platform accordingly.
/james - 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/