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
