Re: [PATCH] ds1337 4/4

[James Chapman <jchapman@katalix.com>]

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